Support for users with red-green colour-blindness
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? |
david | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
W391 blank line at end of file |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
W391 blank line at end of file |
reviewbot | |
Because we will never care about querying on this field, instead of adding a new model field for this, which … |
david | |
W291 trailing whitespace |
reviewbot | |
The first line of docstrings (the summary) should fit on a single line. You can be pretty terse in these … |
david | |
E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
E131 continuation line unaligned for hanging indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
E502 the backslash is redundant between brackets |
reviewbot | |
E131 continuation line unaligned for hanging indent |
reviewbot | |
E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
All your other code assumes the .rg-colorblind class on the body element, but these don't. Can't we use the same … |
david | |
There's trailing whitespace on this line. |
david | |
Variable name should be rgColorblind to match the code style. That said, if we change the stylesheet to respect the … |
david | |
This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code). |
david | |
Can you undo this deletion? |
david | |
For properties, even though they're methods, they should be documented as if they're attributes. So the docstring can just be … |
david | |
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? |
david | |
Can we add a blank line between the color: rule and the .rg-colorblind block? |
david | |
Can we add a blank line between the color: rule and the .rg-colorblind block? |
david | |
Can we add a blank line between the color: rule and the .rg-colorblind block? |
david | |
Can we add a blank line between the color: rule and the .rg-colorblind block? |
david | |
Space between ) and { |
david | |
Can we add a blank line between the .deletebase() macro and the .rg-colorblind block? |
david | |
Need a space between insert and { |
david | |
Can we add a blank line between the .insertbase() macro and the .rg-colorblind block? |
david | |
Can we add a blank line between the .replacebase() macro and the .rg-colorblind block? |
david | |
Can we add a blank line between the background-color: rule and the .rg-colorblind block? |
david | |
Blank line |
david | |
Blank line. |
david | |
This should only be indented 1 space (should match the indentation of reviewable-page) |
david |
- Change Summary:
-
added the new colorblind preference to the database
- Commit:
-
a0b68fdb61ee8b506c22ecafe1399abbeb986df1ec7526ec0e16363ad627588c7e53a932f481d2fb
- Change Summary:
-
added colorblind colors to complexity icons that are assigned according to user setting
- Commit:
-
ec7526ec0e16363ad627588c7e53a932f481d2fbe5894a2a472433f39d7c3396f5b59cfaa43e2db1
- Diff:
-
Revision 3 (+62 -3)
-
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.
-
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
. -
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.
- Change Summary:
-
moved new colorblind setting under profile.settings and also fixed soem style issues
- Commit:
-
e5894a2a472433f39d7c3396f5b59cfaa43e2db121ddc877ea771c69da2b405e9f6ae34e8528a94c
- Diff:
-
Revision 4 (+61 -7)
- Commit:
-
21ddc877ea771c69da2b405e9f6ae34e8528a94c3b09db04c363d1f15eb66f56b2bfcd3307c721cd
- Diff:
-
Revision 5 (+61 -7)
- Commit:
-
3b09db04c363d1f15eb66f56b2bfcd3307c721cdf6445f09c580255efd29073095f21726086a0a78
- Diff:
-
Revision 6 (+60 -7)
- Commit:
-
f6445f09c580255efd29073095f21726086a0a78cd07fbc2e1949c8c48c72aa1fbbf37052aa0ca6f
- Diff:
-
Revision 7 (+60 -7)
Checks run (2 succeeded)
- Commit:
-
cd07fbc2e1949c8c48c72aa1fbbf37052aa0ca6f02439f705c2944b669ef40ef10e5bc73d5c984da
- Diff:
-
Revision 8 (+60 -7)
Checks run (2 succeeded)
- Description:
-
[WIP] 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. ~ 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 and added a colorblind class to be used in diffviewer.
- Change Summary:
-
added new colours for replace and delete
- Commit:
-
02439f705c2944b669ef40ef10e5bc73d5c984da6a6f13967e58db1c89e6977a33e88972a5530741
- Diff:
-
Revision 9 (+106 -11)
Checks run (2 succeeded)
- Description:
-
[WIP] 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 and added a colorblind class to be used in diffviewer.
+ + WIP: pending tests
- Description:
-
~ [WIP] 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.
~ 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 and added a colorblind class to be used in diffviewer.
~ 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 to enable the alternate color scheme.
- - WIP: pending tests
- Testing Done:
-
+ 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.
- Commit:
-
6a6f13967e58db1c89e6977a33e88972a5530741e75066d12e328de980382df841cfcc89443ff63e
- Diff:
-
Revision 10 (+117 -11)
Checks run (2 succeeded)
-
-
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? -
-
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.
-
This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code).
-
- Commit:
-
e75066d12e328de980382df841cfcc89443ff63edaad3889f64b4c2e7b9c4e25779f3fa4565c32cf
Checks run (2 succeeded)
-
Functionality looks good. Just some very minor spacing and documentation things.
-
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."""
-
Indentation is funky here.
Can we add a blank line between the
color:
rule and the.rg-colorblind
block? -
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
added the new colourscheme to the diff viewer within review comments
- Commit:
-
daad3889f64b4c2e7b9c4e25779f3fa4565c32cfc1f7501cd49673595ded20d4fcc3ca85f81dca12
- Diff:
-
Revision 12 (+120 -8)
Checks run (2 succeeded)
- Commit:
-
c1f7501cd49673595ded20d4fcc3ca85f81dca120d6022d4eb0b396381e3d3bd85ba17cad2c6ad48
- Diff:
-
Revision 13 (+114 -8)
Checks run (2 succeeded)
- Description:
-
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 to enable the alternate color scheme.
~ 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.