flake8
passed.
JSHint
passed.
Review Composer: Base Foundation Restructuring
Review Request #10210 — Created Oct. 9, 2018 and updated
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? |
 brennie
|
|
|
Your description mentions this a WIP commit |
 bolariinwa
|
|
|
I noticed in the screenshot the edit icon (pencil) wasn't to the far right as shown in the design document. … |
bolariinwa
|
|
|
Regarding testing done, I think you could run the unit tests for the review dialog to make sure everything still … |
bolariinwa
|
|
|
You can remove the bit about the tests on master failing from your testing done. |
brennie
|
|
|
Class names should be kebab-case e.g. ship-it-checkbox-area. |
brennie
|
|
|
This is needlessly specific. .ship-it-checkbox would be fine. The more specific a selector is, the more specific a selector that … |
brennie
|
|
|
We can restructure this as: .ship-it-checkbox { display: none; &:not(:checked) ~ .rb-icon-admin-enabled { // ... } &:checked ~ .rb-icon-admin-enabled { … |
brennie
|
|
|
Why this change? Isn't this the label for the body top editor? If this is correct, gettext('') will always be … |
brennie
|
|
|
Undo this. |
brennie
|
|
|
Why this change? |
brennie
|
|
|
I don't know that this is the right text for this. |
brennie
|
|
|
You should add a semicolon here |
bolariinwa
|
|
|
You should add a semicolon here |
bolariinwa
|
|
|
Can this float: right style be added to the stylesheet? |
 shoven
|
|
|
In this section you have about four <label> elements for one input (for="id_shipit). While it is technically possible to have … |
shoven
|
Change Summary:
Removed old experimental JS functions.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+82 -79) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Added fixes to the Ship It Checkbox. Improved Comment Interface.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+118 -85) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
I see you've made changes to the visual style, so can you add a screenshot or screenshots showing these changes?
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 3) Class names should be
kebab-casee.g.ship-it-checkbox-area. -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 3) This is needlessly specific.
.ship-it-checkboxwould 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.
-
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 { // ... } } -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 3) Why this change? Isn't this the label for the body top editor?
If this is correct,
gettext('')will always be''. -
-
-
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 3) I don't know that this is the right text for this.
Change Summary:
Fixed the issue with the 'Ship It' checkbox not properly displaying on screen.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+124 -84) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 4) Can this
float: rightstyle be added to the stylesheet? -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 4) 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)
-
-
-
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.
Description: |
|
|---|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+124 -84) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+124 -84) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+124 -84) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+124 -83) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.