• 
      

    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.