Show text message "Loading File attachments..." in the files placeholder while the attachments are loading

Review Request #8642 — Created Jan. 20, 2017 and submitted

anni_cao
Review Board
master
4468
c68def2...
reviewboard, students

When load a review request with some file attachments, the "Files" list appears blank until the attachments finally load in, it looks broken.
After change, a text message "Loading File attachments..." will show in the files placeholder while the attachments are loading.

I have made some testing as following:
1. Created a new review request (File attachment request only), then added a file, after clicking "UPLOAD" button and before a file is loaded, the text message is showing in the files placeholder. Once the file is uploaded, the text message is replaced by the file thumbnail.
2. For an existing request (File attachment request only), when I added a new file, same thing happened as above.

Loading file attachments...

Description From Last Updated

Please put the bug number into the bugs field, and update your summary/description to discuss the change itself. Please see ...

daviddavid

Typo in "Testing Done": "exsitng"

daviddavid

Can you add a screenshot of how this looks with the placeholder there?

chipx86chipx86

Ideally, we'd have a spinner next to the loading text. You can place the following before the text <span class="fa ...

chipx86chipx86

Summary shouldn't list the bug number or "Easyfix". It should just describe the change.

chipx86chipx86

There are too many newlines here.

daviddavid

If you change the ID in the HTML file, make sure to update it here. We also prefer single quotes ...

daviddavid

A few things on this line: Can we use a <div> instead of <span style="display:block">. Can we change the id ...

daviddavid

redefinition of unused 'TestCase' from line 55

reviewbotreviewbot

We won't need the style="display:block" anymore, since <div>'s have display:block by default.

mike_conleymike_conley

We should add aria-hidden="true" to the spinner <span> so screen readers don't say anything aloud. You can also put the ...

daviddavid

There is an extra space between {% and trans.

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. Can you test the case of opening a review request with no file attachments, and then adding a new one via drag-and-drop?

  2. Please put the bug number into the bugs field, and update your summary/description to discuss the change itself. Please see https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for details on what we like to see for those.

  3. Typo in "Testing Done": "exsitng"

  4. There are too many newlines here.

  5. If you change the ID in the HTML file, make sure to update it here.

    We also prefer single quotes in JS strings.

  6. A few things on this line:

    • Can we use a <div> instead of <span style="display:block">.
    • Can we change the id to be review-request-files-placeholder?
    • "Loading file attachments..." should be wrapped in {% trans "..." %}
  7. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/stages/builtin_stages.py
        reviewboard/reviews/evolutions/add_stages.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/tests/test_review_request_stage_controller.py
        reviewboard/datagrids/columns.py
        reviewboard/reviews/__init__.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/stages/__init__.py
        reviewboard/reviews/stages/registry.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/stages/controller.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/datagrids/grids.py
        reviewboard/reviews/stages/base.py
        reviewboard/reviews/tests/test_review_request_draft.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/extensions/tests.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/extensions/tests.py (Diff revision 2)
     
     
     redefinition of unused 'TestCase' from line 55
    
  3. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/stages/builtin_stages.py
        reviewboard/reviews/evolutions/add_stages.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/tests/test_review_request_stage_controller.py
        reviewboard/datagrids/columns.py
        reviewboard/reviews/__init__.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/stages/__init__.py
        reviewboard/reviews/stages/registry.py
        reviewboard/extensions/hooks.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/stages/controller.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/datagrids/grids.py
        reviewboard/reviews/stages/base.py
        reviewboard/reviews/tests/test_review_request_draft.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/extensions/tests.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
AN
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
mike_conley
  1. This looks great, anni! Just one more thing and then I think you're good to go.

  2. We won't need the style="display:block" anymore, since <div>'s have display:block by default.

  3. 
      
chipx86
  1. 
      
  2. Can you add a screenshot of how this looks with the placeholder there?

    1. where should I add the screenshot?

  3. Ideally, we'd have a spinner next to the loading text. You can place the following before the text

    <span class="fa fa-spinner fa-pulse"></span>
    
  4. Summary shouldn't list the bug number or "Easyfix". It should just describe the change.

  5. 
      
AN
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
AN
david
  1. 
      
  2. We should add aria-hidden="true" to the spinner <span> so screen readers don't say anything aloud.

    You can also put the {% trans ... %} on the next line.

    Indentation within HTML files should be 1 space.

  3. 
      
AN
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
brennie
  1. 
      
  2. There is an extra space between {% and trans.

  3. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
AN
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (5a87118)
Loading...