flake8
-
reviewboard/accounts/forms/pages.py (Diff revision 1) Show all issues
Review Request #9251 — Created Oct. 8, 2017 and updated
Users who are red-green colour blind will likely have more difficulty using the diff viewer and reading change descriptions, as we use red for "line removed" and green for "line added". We should have a preference for changing the add & remove colours to another set of colours that are more distinguishable. This preference would apply to anywhere we use this visual metaphor.
A more colourblind-friendly alternative to green is blue for inserts and yellow instead of red for deletes. Because yellow is used by default for replaced lines, replacement lines will be a new shade of blue that appears as a cool grey for colourblind users.I've added a new colorblind setting in user preferences which is saved under the user's profile's settings and added a colorblind class to be applied to the diffviewer (including the diff viewer in review comments) to enable the alternate color scheme.
Added a test under the profile tests of the accounts tests that tests whether the new setting saves to the profile correctly and that the setting is evaluated as false by default.
Description | From | Last Updated |
---|---|---|
One more thing--can you add some screenshots to this review request? |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
W391 blank line at end of file |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
W391 blank line at end of file |
![]() |
|
Because we will never care about querying on this field, instead of adding a new model field for this, which … |
|
|
W291 trailing whitespace |
![]() |
|
The first line of docstrings (the summary) should fit on a single line. You can be pretty terse in these … |
|
|
E251 unexpected spaces around keyword / parameter equals |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E251 unexpected spaces around keyword / parameter equals |
![]() |
|
E502 the backslash is redundant between brackets |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E251 unexpected spaces around keyword / parameter equals |
![]() |
|
All your other code assumes the .rg-colorblind class on the body element, but these don't. Can't we use the same … |
|
|
There's trailing whitespace on this line. |
|
|
Variable name should be rgColorblind to match the code style. That said, if we change the stylesheet to respect the … |
|
|
This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code). |
|
|
Can you undo this deletion? |
|
|
For properties, even though they're methods, they should be documented as if they're attributes. So the docstring can just be … |
|
|
I think this needs a docstring, but confirm with the mentors. Please see the link for details about docstrings. https://www.notion.so/reviewboard/Standard-Documentation-Format-4388f594d86547cc949b365cda3cf391 |
MU mukhtar | |
Indentation is funky here. Can we add a blank line between the color: rule and the .rg-colorblind block? |
|
|
Can we add a blank line between the color: rule and the .rg-colorblind block? |
|
|
Can we add a blank line between the color: rule and the .rg-colorblind block? |
|
|
Can we add a blank line between the color: rule and the .rg-colorblind block? |
|
|
Can we add a blank line between the color: rule and the .rg-colorblind block? |
|
|
Space between ) and { |
|
|
Can we add a blank line between the .deletebase() macro and the .rg-colorblind block? |
|
|
Need a space between insert and { |
|
|
Can we add a blank line between the .insertbase() macro and the .rg-colorblind block? |
|
|
Can we add a blank line between the .replacebase() macro and the .rg-colorblind block? |
|
|
Can we add a blank line between the background-color: rule and the .rg-colorblind block? |
|
|
Blank line |
|
|
Blank line. |
|
|
This should only be indented 1 space (should match the indentation of reviewable-page) |
|
added the new colorblind preference to the database
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+39 -2) |
reviewboard/accounts/evolutions/profile_colorblind_scheme.py (Diff revision 2) |
---|
E501 line too long (81 > 79 characters)
reviewboard/accounts/evolutions/profile_colorblind_scheme.py (Diff revision 2) |
---|
W391 blank line at end of file
added colorblind colors to complexity icons that are assigned according to user setting
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+62 -3) |
reviewboard/accounts/evolutions/profile_colorblind_scheme.py (Diff revision 3) |
---|
E501 line too long (81 > 79 characters)
reviewboard/accounts/evolutions/profile_colorblind_scheme.py (Diff revision 3) |
---|
W391 blank line at end of file
When making changes to the UI, it's very useful to have screenshots to look at. The best for something like this is to have two: before and after.
reviewboard/accounts/models.py (Diff revision 3) |
---|
Because we will never care about querying on this field, instead of adding a new model field for this, which requires a database evolution, it would be nice to stick the value into the
extra_data
field (which works a lot like a Python dictionary but serializes to JSON).All the other things in here that are model fields existed before we added
extra_data
.
reviewboard/accounts/models.py (Diff revision 3) |
---|
The first line of docstrings (the summary) should fit on a single line. You can be pretty terse in these and then flesh out the meaning with more paragraphs below.
moved new colorblind setting under profile.settings and also fixed soem style issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+61 -7) |
reviewboard/accounts/forms/pages.py (Diff revision 4) |
---|
E251 unexpected spaces around keyword / parameter equals
reviewboard/accounts/forms/pages.py (Diff revision 4) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/accounts/models.py (Diff revision 4) |
---|
E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+61 -7) |
reviewboard/accounts/forms/pages.py (Diff revision 5) |
---|
E251 unexpected spaces around keyword / parameter equals
reviewboard/accounts/forms/pages.py (Diff revision 5) |
---|
E502 the backslash is redundant between brackets
reviewboard/accounts/forms/pages.py (Diff revision 5) |
---|
E131 continuation line unaligned for hanging indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+60 -7) |
reviewboard/accounts/forms/pages.py (Diff revision 6) |
---|
E251 unexpected spaces around keyword / parameter equals
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+60 -7) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+60 -7) |
Description: |
|
---|
added new colours for replace and delete
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+106 -11) |
Description: |
|
---|
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 10 (+117 -11) |
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 10) |
---|
All your other code assumes the
.rg-colorblind
class on the body element, but these don't. Can't we use the same mechanism here instead of changing the classes on the elements?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 10) |
---|
There's trailing whitespace on this line.
reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js (Diff revision 10) |
---|
Variable name should be rgColorblind to match the code style. That said, if we change the stylesheet to respect the body's .rg-colorblind class we don't need any of this.
reviewboard/templates/base.html (Diff revision 10) |
---|
This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code).
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+102 -7) |
Functionality looks good. Just some very minor spacing and documentation things.
reviewboard/accounts/models.py (Diff revision 11) |
---|
For properties, even though they're methods, they should be documented as if they're attributes. So the docstring can just be a single line:
"""Whether to use a colorblid-safe theme for this user."""
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Indentation is funky here.
Can we add a blank line between the
color:
rule and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
color:
rule and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
color:
rule and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
color:
rule and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
color:
rule and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
.deletebase()
macro and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Need a space between
insert
and{
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
.insertbase()
macro and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
.replacebase()
macro and the.rg-colorblind
block?
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) |
---|
Can we add a blank line between the
background-color:
rule and the.rg-colorblind
block?
reviewboard/templates/reviews/reviewable_base.html (Diff revision 11) |
---|
This should only be indented 1 space (should match the indentation of
reviewable-page
)
reviewboard/accounts/forms/pages.py (Diff revision 11) |
---|
Is it just the diff viewer that would become colorblind-friendly? Does it also change the colors in the "Reviews" section?
reviewboard/accounts/models.py (Diff revision 11) |
---|
I think this needs a docstring, but confirm with the mentors. Please see the link for details about docstrings.
https://www.notion.so/reviewboard/Standard-Documentation-Format-4388f594d86547cc949b365cda3cf391
reviewboard/accounts/tests.py (Diff revision 11) |
---|
I don't know how thorough you want to be with testing, but you could also login as the user with updated colorblind settings and then request a view and verify the class is applied with
self.assertContains(response, "rg-colorblind")
.
added the new colourscheme to the diff viewer within review comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+120 -8) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+114 -8) |
Description: |
|
---|