Review Composer: Base Foundation Restructuring

Review Request #10210 — Created Oct. 9, 2018 and updated

Information

Review Board
master
e1476b7...

Reviewers

In this commit, the references to ReviewDialog
in the codebase were renamed to ReviewComposer.
The exceptions to this are the ReviewDialog references
that have to do with extensions. There were slight
format changes made to the header and footer. The
icon for the ShipIt checkbox was added but there is
some CSS that needs to be added in to make the toggling
between checked/unchecked icons to work. The comments
also still need to be restructured.

I ran the Javascript Unit tests. The 6 test failures are the same as the ones in the master branch so the changes seems to have not broken anything.


Description From Last Updated

I see you've made changes to the visual style, so can you add a screenshot or screenshots showing these changes?

brenniebrennie

Your description mentions this a WIP commit

bolariinwabolariinwa

I noticed in the screenshot the edit icon (pencil) wasn't to the far right as shown in the design document. …

bolariinwabolariinwa

Regarding testing done, I think you could run the unit tests for the review dialog to make sure everything still …

bolariinwabolariinwa

You can remove the bit about the tests on master failing from your testing done.

brenniebrennie

Class names should be kebab-case e.g. ship-it-checkbox-area.

brenniebrennie

This is needlessly specific. .ship-it-checkbox would be fine. The more specific a selector is, the more specific a selector that …

brenniebrennie

We can restructure this as: .ship-it-checkbox { display: none; &:not(:checked) ~ .rb-icon-admin-enabled { // ... } &:checked ~ .rb-icon-admin-enabled { …

brenniebrennie

Why this change? Isn't this the label for the body top editor? If this is correct, gettext('') will always be …

brenniebrennie

Undo this.

brenniebrennie

Why this change?

brenniebrennie

I don't know that this is the right text for this.

brenniebrennie

You should add a semicolon here

bolariinwabolariinwa

You should add a semicolon here

bolariinwabolariinwa

Can this float: right style be added to the stylesheet?

shovenshoven

In this section you have about four <label> elements for one input (for="id_shipit). While it is technically possible to have …

shovenshoven
Malcolm
Malcolm
brennie
  1. 
      
  2. Show all issues

    I see you've made changes to the visual style, so can you add a screenshot or screenshots showing these changes?

    1. I have added some pictures and screenshots of the old Review Dialog Interface, the Review Composer Design document, and the current state of the Review Composer.

  3. Show all issues

    Class names should be kebab-case e.g. ship-it-checkbox-area.

  4. Show all issues

    This is needlessly specific. .ship-it-checkbox would be fine.

    The more specific a selector is, the more specific a selector that would override it must be, which can lead to unmaintainable CSS.

  5. reviewboard/static/rb/css/pages/reviews.less (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We can restructure this as:

    .ship-it-checkbox {
      display: none;
    
      &:not(:checked) ~ .rb-icon-admin-enabled {
        // ...
      }
    
      &:checked ~ .rb-icon-admin-enabled {
        // ...
      }
    
      &:checked ~ .rb-icon-admin-disabled {
        // ...
      }
    }
    
  6. Show all issues

    Why this change? Isn't this the label for the body top editor?

    If this is correct, gettext('') will always be ''.

    1. The design document show that there is no text displayed for the Header and Footer. I removed this text here to get that same effect. I'm not sure if the Header text should be different or completely removed.

  7. Show all issues

    Undo this.

  8. Show all issues

    Why this change?

    1. The reasons mentioned in the previous section about the Header text holds for the Footer text as well.

  9. Show all issues

    I don't know that this is the right text for this.

    1. This is the text that was displayed in the Review Composer design document. I have uploaded the design document in this review request.

  10. 
      
Malcolm
Malcolm
  1. 
      
  2. 
      
Malcolm
Malcolm
shoven
  1. 
      
  2. Show all issues

    Can this float: right style be added to the stylesheet?

  3. Show all issues

    In this section you have about four <label> elements for one input (for="id_shipit). While it is technically possible to have multiple labels for one form element, there are some accessibility concerns with this as many assistive technologies, like screenreaders, don't know how to handle them.

    I'm not sure what the best approach is in this particular case so you might need to do some playing so you only have one for= for the one input. (There's an example here that could be helpful: https://developer.mozilla.org/en-US/docs/Learn/HTML/Forms/How_to_structure_an_HTML_form#Multiple_labels)

    1. I will attempt a fix for this in the Review Composer: Comment UI review request.

  4. 
      
bolariinwa
  1. 
      
  2. Show all issues

    Your description mentions this a WIP commit

  3. Show all issues

    Regarding testing done, I think you could run the unit tests for the review dialog to make sure everything still works fine. There were also variable name changes in the test files so it will be good to see that the tests still run properly.
    If you feel like your changes do not need a unit test, you could just state what you manually tested and how it works fine.

    1. Good point. I have rerun the tests and the failures are the same ones as in the master branch.

  4. 
      
bolariinwa
  1. 
      
  2. Show all issues

    I noticed in the screenshot the edit icon (pencil) wasn't to the far right as shown in the design document. You can find change the position in the inlineEditorView.es6.js, if you search for class="editicon" you should find the element you need to get the desired effect

    1. Thanks, I found it and fixed it in the Comment UI review request.

  3. 
      
bolariinwa
  1. 
      
  2. Show all issues

    You should add a semicolon here

  3. Show all issues

    You should add a semicolon here

  4. 
      
Malcolm
Malcolm
Malcolm
Malcolm
Malcolm
Malcolm
Review request changed
brennie
  1. 
      
  2. Show all issues

    You can remove the bit about the tests on master failing from your testing done.

  3.