Summary:

Modified styling for diff viewer lines to be colorblind-friendly

Review Request #10981 — Created March 29, 2020 and updated April 9, 2020, 5:34 p.m.

Information
Owner:	kpatenio
Repository:	Review Board
Branch:	release-4.0.x
Bugs:
Depends On:
Commit:	87859c5...
Reviewers
Groups:	reviewboard
People:

Description

The red, green, and yellow colors used in Review Board's diff viewer aren't
easily dintinguishable for users with various colorblind disabilities.

To address this, icons have been added to inserted, replaced, and deleted
diff viewer lines (+, ~, and - respectively) to visually represent the type
of change made without having to solely rely on color. Alignment for equal
lines (unmodified lines of code) was also updated; line numbers for all
diff types should now be aligned.

I've attached a video below showing how the diff viewer would like with my
new changes.

For more information on how I came up with these changes, see my notes here.

Testing Done

All current unit tests pass. No additional tests were made.

Files

Issues

Description	From	Last Updated
Could we perhaps reuse some of the builtin colors already defined in reviewboard/reviewboard/static/rb/css/ui/colors.less? For example, #rb-ns-ui.colors[@red-60]; You'll just need to …	bui1	April 4, 2020, 12:05 a.m.
Same comment here about reusing colors #rb-ns-ui.colors[@green-70];	bui1	April 4, 2020, 12:05 a.m.
Same comment here about colors #rb-ns-ui.colors[@yellow-90];. I know the shades of yellow here are limited so if they don't work …	bui1	April 4, 2020, 12:05 a.m.
I think this comment should use /* ... */	hxqlin	April 8, 2020, 2:53 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Make a .less mixin to be used by all diff lines + uploaded more screenshots

Commit:

-	1c4e4451550a2ea7b32f01a6b02ddc8500c4081d
+	283619d65c6ba12bcfc37ef6165a411823652539

Diff:

Revision 2 (+20)

Show changes

reviewboard/static/rb/css/pages/diffviewer.less

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The screenshots look good to me! The icons themselves are easier to see given the size and boldness.

Just another question (maybe since I'm not sure of the current state of the project), did we scrap the idea of changing colors altogether per mode of colorblindness?

kpatenio April 4, 2020, 12:05 a.m.

Sorry the delayed response! I believe we did scrap the idea of changing colours. If we do decide to do that, I'd ideally like to do that change in another request if possible.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)

Show all issues

Could we perhaps reuse some of the builtin colors already defined in reviewboard/reviewboard/static/rb/css/ui/colors.less?

For example,
#rb-ns-ui.colors[@red-60];

You'll just need to play around with the different shades of red to get the best one you need for the job

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
Show all issues
```
Same comment here about reusing colors
#rb-ns-ui.colors[@green-70];
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)

Show all issues

Same comment here about colors
#rb-ns-ui.colors[@yellow-90];.

I know the shades of yellow here are limited so if they don't work out, I think leaving it as is should ok or even defining it as a variable for future reusability is also a good idea.

Change Summary:

Utilized colors.less mixin for colour values

Commit:

-	283619d65c6ba12bcfc37ef6165a411823652539
+	1fa7fa806e31f61886762b0a4f6c3b0b9923d001

Diff:

Revision 3 (+25 -1)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/ui/colors.less

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This looks pretty good! One another suggestion is to add a short comment of what the attached screenshots are for and possibly updating their captions. For a small change like this, it's relatively obvious that they're showing the UI changes but in a review request that might have multiple attached files to explain different parts of the change, it might not be so clear.

hxqlin April 5, 2020, 8:31 p.m.

Oops just to clarify the add a short comment bit - I meant for this to be added in the description.

kpatenio April 8, 2020, 2:54 p.m.

Thanks for your feedback Hannah! I've updated the description to more clear. :)

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)
Show all issues
```
I think this comment should use /* ... */
```

Change Summary:

Updated description to be more clear + fixed comment styling

Description:

		The red, green, and yellow colors used in Review Board's diff viewer aren't
		easily dintinguishable for users with various colorblind disabilities.

		To address this, icons have been added to inserted, replaced, and deleted
		diff viewer lines (+, ~, and - respectively) to visually represent the type
		of change made without having to solely rely on color.

	+	For a visual representation of these changes, I've attached screenshots
	+	below. I chose screenshots showing the three different types of diffs,
	+	both as multi-line changes and as single line changes, in order to
	+	demonstrate the readability of the diffs with the newly added icons.
	+
		For more information, see my notes here.

Commit:

-	1fa7fa806e31f61886762b0a4f6c3b0b9923d001
+	d0dd941b463f8e75538c94638648c4eac10ef295

Diff:

Revision 4 (+27 -1)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/ui/colors.less

File Captions:

Screen Shot 2020-03-29 at 16.10.57.png:

-	Screen Shot 2020-03-29 at 16.10.57.png
+	Multiline "modified" (yellow) diff changes.

Screen Shot 2020-03-29 at 16.11.19.png:

-	Screen Shot 2020-03-29 at 16.11.19.png
+	Multiline "insert" (green) and "deleted" (red) diff changes.

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Updated styling to be consistent + more aethestically pleasing

Description:

		The red, green, and yellow colors used in Review Board's diff viewer aren't
		easily dintinguishable for users with various colorblind disabilities.

		To address this, icons have been added to inserted, replaced, and deleted
		diff viewer lines (+, ~, and - respectively) to visually represent the type
		of change made without having to solely rely on color.

		For a visual representation of these changes, I've attached screenshots
~		below. I chose screenshots showing the three different types of diffs,
~		both as multi-line changes and as single line changes, in order to
	~	below. I chose screenshots showing the three different types of diffs; this
	~	is to demonstrate the readability of the diffs with the newly added icons.
-		demonstrate the readability of the diffs with the newly added icons.

		For more information, see my notes here.

Commit:

-	d0dd941b463f8e75538c94638648c4eac10ef295
+	855ea882c2535ad4de4470fe3523d61df2f78267

Diff:

Revision 5 (+42)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/ui/colors.less

Removed Files:

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fixed alignment issue for line numbers of equal diffs + rebased with release 4

Commit:

-	855ea882c2535ad4de4470fe3523d61df2f78267
+	87859c54896e9ca65be247161f8cf3657b9521e8

Diff:

Revision 6 (+44)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/ui/colors.less

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added video showing changes

Removed Files:
Added Files:

Change Summary:

Updated description

Description:

		The red, green, and yellow colors used in Review Board's diff viewer aren't
		easily dintinguishable for users with various colorblind disabilities.

		To address this, icons have been added to inserted, replaced, and deleted
		diff viewer lines (+, ~, and - respectively) to visually represent the type
~		of change made without having to solely rely on color.
	~	of change made without having to solely rely on color. Alignment for equal
	+	lines (unmodified lines of code) was also updated; line numbers for all
	+	diff types should now be aligned.

~		For a visual representation of these changes, I've attached screenshots
~		below. I chose screenshots showing the three different types of diffs; this
	~	I've attached a video below showing how the diff viewer would like with my
	~	new changes.
-		is to demonstrate the readability of the diffs with the newly added icons.

~		For more information, see my notes here.
	~	For more information on how I came up with these changes, see my notes here.