• 
      

    Support for users with red-green colour-blindness

    Review Request #9251 — Created Oct. 8, 2017 and updated

    Information

    Review Board
    release-3.0.x
    0d6022d...

    Reviewers

    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?

    daviddavid

    E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    W391 blank line at end of file

    reviewbotreviewbot

    E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    W293 blank line contains whitespace

    reviewbotreviewbot

    E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    W391 blank line at end of file

    reviewbotreviewbot

    Because we will never care about querying on this field, instead of adding a new model field for this, which …

    daviddavid

    W291 trailing whitespace

    reviewbotreviewbot

    The first line of docstrings (the summary) should fit on a single line. You can be pretty terse in these …

    daviddavid

    E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    E131 continuation line unaligned for hanging indent

    reviewbotreviewbot

    E128 continuation line under-indented for visual indent

    reviewbotreviewbot

    E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    E502 the backslash is redundant between brackets

    reviewbotreviewbot

    E131 continuation line unaligned for hanging indent

    reviewbotreviewbot

    E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    All your other code assumes the .rg-colorblind class on the body element, but these don't. Can't we use the same …

    daviddavid

    There's trailing whitespace on this line.

    daviddavid

    Variable name should be rgColorblind to match the code style. That said, if we change the stylesheet to respect the …

    daviddavid

    This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code).

    daviddavid

    Can you undo this deletion?

    daviddavid

    For properties, even though they're methods, they should be documented as if they're attributes. So the docstring can just be …

    daviddavid

    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?

    daviddavid

    Can we add a blank line between the color: rule and the .rg-colorblind block?

    daviddavid

    Can we add a blank line between the color: rule and the .rg-colorblind block?

    daviddavid

    Can we add a blank line between the color: rule and the .rg-colorblind block?

    daviddavid

    Can we add a blank line between the color: rule and the .rg-colorblind block?

    daviddavid

    Space between ) and {

    daviddavid

    Can we add a blank line between the .deletebase() macro and the .rg-colorblind block?

    daviddavid

    Need a space between insert and {

    daviddavid

    Can we add a blank line between the .insertbase() macro and the .rg-colorblind block?

    daviddavid

    Can we add a blank line between the .replacebase() macro and the .rg-colorblind block?

    daviddavid

    Can we add a blank line between the background-color: rule and the .rg-colorblind block?

    daviddavid

    Blank line

    daviddavid

    Blank line.

    daviddavid

    This should only be indented 1 space (should match the indentation of reviewable-page)

    daviddavid
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    CL
    Review request changed
    Change Summary:

    added the new colorblind preference to the database

    Commit:
    a0b68fdb61ee8b506c22ecafe1399abbeb986df1
    ec7526ec0e16363ad627588c7e53a932f481d2fb

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    CL
    Review request changed
    Change Summary:

    added colorblind colors to complexity icons that are assigned according to user setting

    Commit:
    ec7526ec0e16363ad627588c7e53a932f481d2fb
    e5894a2a472433f39d7c3396f5b59cfaa43e2db1

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

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

    2. reviewboard/accounts/models.py (Diff revision 3)
       
       
       
       
       
       
      Show all issues

      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.

    3. reviewboard/accounts/models.py (Diff revision 3)
       
       
       
      Show all issues

      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.

    4. 
        
    CL
    Review request changed
    Change Summary:

    moved new colorblind setting under profile.settings and also fixed soem style issues

    Commit:
    e5894a2a472433f39d7c3396f5b59cfaa43e2db1
    21ddc877ea771c69da2b405e9f6ae34e8528a94c

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    CL
    Review request changed
    Commit:
    21ddc877ea771c69da2b405e9f6ae34e8528a94c
    3b09db04c363d1f15eb66f56b2bfcd3307c721cd

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    CL
    Review request changed
    Commit:
    3b09db04c363d1f15eb66f56b2bfcd3307c721cd
    f6445f09c580255efd29073095f21726086a0a78

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    CL
    CL
    CL
    CL
    CL
    CL
    CL
    david
    1. 
        
    2. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 10)
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      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?

    3. Show all issues

      There's trailing whitespace on this line.

    4. Show all issues

      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.

    5. reviewboard/templates/base.html (Diff revision 10)
       
       
      Show all issues

      This variable should be "rgColorblindScheme" to match the casing on the other nearby ones (we use camelCase within javascript code).

    6. Show all issues

      Can you undo this deletion?

    7. 
        
    david
    1. 
        
    2. Show all issues

      One more thing--can you add some screenshots to this review request?

      1. How do I do this? Do I need to put them on some sort of image hosting site?

      2. Just go to "Update > Add File" at the top of the review request box.

    3. 
        
    CL
    CL
    david
    1. Functionality looks good. Just some very minor spacing and documentation things.

    2. reviewboard/accounts/models.py (Diff revision 11)
       
       
       
       
       
       
       
      Show all issues

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

      1. I was following what was done for the "should_enable_desktop_notifications" property that is right above it. it had a docstring like:

            """Return whether desktop notifications should be used for this user.
        
            If the user has chosen whether or not to use desktop notifications
            explicitly, then that choice will be respected. Otherwise, we
            enable desktop notifications by default.
        
            Returns:
                bool:
                    If the user has set whether they wish to recieve desktop
                    notifications, then use their preference. Otherwise, we return
                    ``True``.
            """
        

        But, I have no problem changing it to one line

    3. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)
       
       
       
       
       
      Show all issues

      Indentation is funky here.

      Can we add a blank line between the color: rule and the .rg-colorblind block?

    4. Show all issues

      Can we add a blank line between the color: rule and the .rg-colorblind block?

    5. Show all issues

      Can we add a blank line between the color: rule and the .rg-colorblind block?

    6. Show all issues

      Can we add a blank line between the color: rule and the .rg-colorblind block?

    7. Show all issues

      Can we add a blank line between the color: rule and the .rg-colorblind block?

    8. Show all issues

      Space between ) and {

    9. Show all issues

      Can we add a blank line between the .deletebase() macro and the .rg-colorblind block?

    10. Show all issues

      Need a space between insert and {

    11. Show all issues

      Can we add a blank line between the .insertbase() macro and the .rg-colorblind block?

    12. Show all issues

      Can we add a blank line between the .replacebase() macro and the .rg-colorblind block?

    13. Show all issues

      Can we add a blank line between the background-color: rule and the .rg-colorblind block?

    14. Show all issues

      Blank line

    15. Show all issues

      Blank line.

    16. Show all issues

      This should only be indented 1 space (should match the indentation of reviewable-page)

    17. 
        
    giuliacm
    1. 
        
    2. 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?

      1. good catch. Originally it was just the diff viewer. I've added in the new scheme for the review comment diff now.

    3. 
        
    MU
    1. 
        
    2. reviewboard/accounts/models.py (Diff revision 11)
       
       
      Show all issues

      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

      1. David mentioned in a review a few above yours that it probably will be a one line docstring

    3. 
        
    TB
    1. 
        
    2. 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").

      1. Do you have any examples on how to go about requesting a view?

      2. I do it in some of my tests here: https://reviews.reviewboard.org/r/9364/diff/14#3

        If you just search through the testing codebase for self.client.get you'll find other examples though.

    3. 
        
    CL
    CL
    CL
    Review request changed
    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.