Rewrite file attachment javascript templating.

Review Request #3587 — Created Nov. 29, 2012 and submitted

david
Review Board
release-1.7.x
reviewboard
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 …

chipx86chipx86

Should be able to do .children('br'), I think.

chipx86chipx86

Like above, if we can have a static variable that we set only once (probably after first call), this will …

chipx86chipx86

Let's just test truthiness, in case this ends up normalized to null or something.

chipx86chipx86

I feel like this all belongs in the template. You can do: <% if blah { %> ... <% } …

chipx86chipx86

Same.

chipx86chipx86

Needs to be a var.

chipx86chipx86

var

chipx86chipx86

Should be: /* * */

chipx86chipx86
chipx86
  1. 
      
  2. 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('')
  3. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Should be able to do .children('br'), I think.
  4. 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.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Let's just test truthiness, in case this ends up normalized to null or something.
  6. 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.
  7. 
      
david
chipx86
  1. Three small things, then I'm fine with it.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
    Needs to be a var.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
     
    Should be:
    
    /*
     *
     */
  4. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.7.x (035b066).
Loading...