-
-
reviewboard/static/rb/js/reviews.js (Diff revision 1) This would be better as an underscore template cached outside the function, so it's compiled only once. When doing lots of string concatenations in JS, it's best to do: [ '...', '...' ].join('') -
reviewboard/static/rb/js/reviews.js (Diff revision 1) Should be able to do .children('br'), I think. -
reviewboard/static/rb/js/reviews.js (Diff revision 1) Like above, if we can have a static variable that we set only once (probably after first call), this will be nicer. And same comment about concatenation.
-
reviewboard/static/rb/js/reviews.js (Diff revision 1) Let's just test truthiness, in case this ends up normalized to null or something.
-
reviewboard/static/rb/js/reviews.js (Diff revision 1) I feel like this all belongs in the template. You can do: <% if blah { %> ... <% } %> The nice thing about that is, when we move to require.js, templates like these can be stored in their own template files and we have a nice separation of templates and logic. -
Rewrite file attachment javascript templating.
Review Request #3587 — Created Nov. 29, 2012 and submitted
Rewrite file attachment javascript templating. When uploading file attachments through the web UI, the box shown on the page would be created from javascript instead of from a django template. The code that did this wasn't quite at parity with the html that was generated when loading the page to begin with. Most annoyingly, it didn't have a link to the review UI (if one existed). It also didn't do any of the new caption stuff that I added. The code now uses _.template for the full case, and simplifies the placeholder case (and fixes it to actually contain a spinner).
- Uploaded a bunch of files, both through the "Add file" dialog and DnD. - Verified that the "Review" link was correct when there was a review UI. - Checked that the generated HTML was the same as when reloading the page. - Commented out the code that replaces the placeholder and verifiend the appearance of the spinner.
| Description | From | Last Updated |
|---|---|---|
|
This would be better as an underscore template cached outside the function, so it's compiled only once. When doing lots … |
 chipx86
|
|
|
Should be able to do .children('br'), I think. |
chipx86
|
|
|
Like above, if we can have a static variable that we set only once (probably after first call), this will … |
chipx86
|
|
|
Let's just test truthiness, in case this ends up normalized to null or something. |
chipx86
|
|
|
I feel like this all belongs in the template. You can do: <% if blah { %> ... <% } … |
chipx86
|
|
|
Same. |
chipx86
|
|
|
Needs to be a var. |
chipx86
|
|
|
var |
chipx86
|
|
|
Should be: /* * */ |
chipx86
|
Change Summary:
Make requested changes. I'm a little unsure about the "caching outside of function" stuff so please let me know if there's a better way.
Diff: |
Revision 2 (+78 -62) |
|---|