Review Composer: Base Foundation Restructuring

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

Malcolm
Review Board
master
e1476b7...
reviewboard, students

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.

Loading file attachments...

  • 5
  • 0
  • 11
  • 0
  • 16
Description From Last Updated
You can remove the bit about the tests on master failing from your testing done. brennie brennie
Why this change? Isn't this the label for the body top editor? If this is correct, gettext('') will always be ... brennie brennie
Why this change? brennie brennie
I don't know that this is the right text for this. brennie brennie
In this section you have about four <label> elements for one input (for="id_shipit). While it is technically possible to have ... shoven shoven
Malcolm
Malcolm
brennie
  1. 
      
  2. 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. Class names should be kebab-case e.g. ship-it-checkbox-area.

  4. 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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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. 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.

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

  7. 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.

  8. 
      
Malcolm
Malcolm
  1. 
      
  2. 
      
Malcolm
Malcolm
shoven
  1. 
      
  2. Can this float: right style be added to the stylesheet?

  3. 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. Your description mentions this a WIP commit

  3. 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. 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. You should add a semicolon here

  3. You should add a semicolon here

  4. 
      
Malcolm
Malcolm
Malcolm
Malcolm
Malcolm
Malcolm
Review request changed
brennie
  1. 
      
  2. You can remove the bit about the tests on master failing from your testing done.

  3. 
      
Loading...