Archive review requests from inbox icon

Review Request #10154 — Created Sept. 21, 2018 and submitted

Information

Review Board
master
9afd7cf...

Reviewers

Made the inbox icon on review request pages a shortcut for
archiving/unarchiving the review request.

The idea behind this change is to make the archiving/unarchiving of
requests a little bit faster for advanced users.

The shortcut was implemented by adding archive/unarchive click handlers
to the icon. A title attribute was also added to the icon which
dynamically changes to "Archive review request" or "Unarchive review
request". This should help with accessibility and general ease of use.

There were no changes made to any Python files so the Python tests were
not ran for this change.

The JS tests were ran for these changes.
These changes were implemented off the master branch version of
ReviewBoard which had 6 failures for the JS tests.

The JS tests when ran on this update had the same 6 failures as in the
master branch JS tests.

Description From Last Updated

Can you add some title= text to the inbox icon that says "Archive" ?

brenniebrennie

Please update the description to include a little bit more information about the motivation for this change.

daviddavid

Please fill out the "Testing done" field explaining what you have done to test the change.

daviddavid

Can you limit your summary to 50 characters or so?

brenniebrennie

Can you rewrite your summary to be in the imperitive mood, i.e., as if it were a command or order? …

brenniebrennie

Can you wrap your description & testing done at 72 characters?

brenniebrennie

This is an es6 file, so we can (and should) have a comma on the last item in the object.

daviddavid

Your code has trailing whitespace. You'll want to remove this.

brenniebrennie

This should be let over `var.

brenniebrennie

Instead of switching on the iconClass, I would do what the calculation for iconClass does, e.g.: const iconTitle = (visibility …

brenniebrennie

These need to use gettext('...') so that the strings are marked for translation.

brenniebrennie

Since these are classes, they aren't necessarily unique. It is possible that the rb-icon-archive-{on,off} class could be used for another …

brenniebrennie

We're now doing the same equality check three times. How about adding: const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE);

daviddavid
brennie
  1. 
      
  2. Show all issues

    Can you add some title= text to the inbox icon that says "Archive" ?

  3. 
      
david
  1. 
      
  2. Show all issues

    Please update the description to include a little bit more information about the motivation for this change.

  3. Show all issues

    Please fill out the "Testing done" field explaining what you have done to test the change.

  4. Show all issues

    This is an es6 file, so we can (and should) have a comma on the last item in the object.

  5. 
      
Malcolm
  1. nn

  2. 
      
Malcolm
Malcolm
Malcolm
brennie
  1. 
      
  2. Show all issues

    Can you limit your summary to 50 characters or so?

  3. Show all issues

    Can you rewrite your summary to be in the imperitive mood, i.e., as if it were a command or order?

    If you substitute your summary into the following sentence, it should make sense:

    This patch will <summary>
    
  4. Show all issues

    Can you wrap your description & testing done at 72 characters?

  5. reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Your code has trailing whitespace. You'll want to remove this.

  6. Show all issues

    This should be let over `var.

  7. Show all issues

    Instead of switching on the iconClass, I would do what the calculation for iconClass does, e.g.:

    const iconTitle = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE
                       ? gettext('Archive review request')
                       : gettext('Unarchive review request'));
    

    This is because the underlying case is a boolean, which are easier to compare compared to strings (which at worst compare N bytes of an N byte string instead of 1).

    Also, at first glance, there is no default case so iconTitle could end up undefined.

  8. Show all issues

    These need to use gettext('...') so that the strings are marked for translation.

  9. 
      
Malcolm
brennie
  1. 
      
  2. Show all issues

    Since these are classes, they aren't necessarily unique. It is possible that the rb-icon-archive-{on,off} class could be used for another icon somewhere in the editor view.

    So let's make them unique by adding an ID to this.

  3. 
      
Malcolm
david
  1. 
      
  2. reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We're now doing the same equality check three times. How about adding:

    const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE);
    
  3. 
      
david
  1. Ship It!
  2. 
      
Malcolm
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (dbd469f)