Summary

Change linenum color of diff viewer for better user visibility.

Review Request #7687 — Created Oct. 9, 2015 and submitted Oct. 30, 2015, 1:06 p.m.

Information

Owner

eklau

Repository

Review Board

Branch

release-2.0.x

Bugs

3981

Depends On

Reviewers

Groups

reviewboard

People

Description

I have changed the line number color of diff viewer for better user visibility. The color in diff viewer before was black by default and was a distraction to the actual code. I have made this line number color lighter and less noticable so that the code is easier to read.

Testing Done

Reloaded the page to see if the color had changed.
Fixed the order of code in diffviewer.less
Updated the diff-line-num to #444444

Files

Issues

Description	From	Last Updated
Hmm. This color might still be too low contrast. Let's darken it a bit.	david	Oct. 15, 2015, 7:03 p.m.
This is the same as after. I think you uploaded the wrong image because thats supposed to be black.	brennie	Oct. 15, 2015, 9:50 p.m.
Instead of using transparency, can we just make this a medium grey color? Something like #777777?	david	Oct. 15, 2015, 6:30 p.m.
We should probably also add font-weight: normal to avoid the browser bolding everything.	david	Oct. 15, 2015, 6:31 p.m.
Should be in alphabetical order.	chipx86	Oct. 24, 2015, 6:14 p.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less

```
Please attach before/after screenshots.
```

Added Files:

david Oct. 12, 2015, 5:41 p.m.

Hey Edward,

If you've addressed the issues below (and it looks like you have), please click "Fixed" on them. We're unlikely to give your changes more rounds of review if we see they still have open issues.

reviewboard/static/rb/css/defs.less (Diff revision 1)

Show all issues

Instead of using transparency, can we just make this a medium grey color? Something like #777777?

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

Show all issues

We should probably also add font-weight: normal to avoid the browser bolding everything.

Diff:

Revision 2 (+5 -2)

Show changes

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

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less

Removed Files:
Added Files:

Your description still talks about transparency, but we're not using transparency anymore.

Please also attach screenshots of equals, removed, and changed blocks (so we can see what it looks like against all color backgrounds).

diffviewafter.png (Revision 1)

Show all issues

Hmm. This color might still be too low contrast. Let's darken it a bit.

Diff:

Revision 3 (+5 -2)

Show changes

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

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less

Removed Files:
Added Files:

diffviewbefore.png (Revision 1)

Show all issues

This is the same as after. I think you uploaded the wrong image because thats supposed to be black.

eklau Oct. 15, 2015, 9:50 p.m.

They are similar but not the same, the before was #777777 that was David wanted darker, so I've changed it to #666666

david Oct. 16, 2015, 1:34 p.m.

The difference between 777777 and 666666 is pretty subtle. Let's try 444444.

Your summary needs to be updated. Other than that, it looks good to me, pending David's approval.

reviewboard/static/rb/css/diffviewer.less (Diff revision 3)
Show all issues
```
Should be in alphabetical order.
```

Testing Done:

~		Reloaded the page to see if the color had changed.
	~	Reloaded the page to see if the color had changed.
	+	Fixed the order of code in diffviewer.less
	+	Updated the diff-line-num to #444444

Removed Files:

Added Files:

Ship it!

```
This looks good to me - thanks!
```

Edward, I'm still waiting on you updating the description to be accurate (we're not using transparency anymore).

EK

eklau Oct. 26, 2015, 1:36 p.m.
```
Ok, I have made that change.
```

Description:

~		I change linenum color of diff viewer for better user visibility. Before, the line number color in diff viewer were all black and it wasn't as user friendly. I increased the transparency and made the color lighter.
	~	I have changed the line number color of diff viewer for better user visibility. The color in diff viewer before was black by default and was not as user friendly. I have made this line number color lighter so that it is easier for the user to notice.

Diff:

Revision 4 (+5 -2)

Show changes

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

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/diffviewer.less

I don't agree with this new description of the change. Making the line numbers lighter weight and a lighter color makes them less noticible, not more. The initial problem was that the heavy-weight numbers distracted from the real focus of the page, which should be the code itself.

Description:

~		I have changed the line number color of diff viewer for better user visibility. The color in diff viewer before was black by default and was not as user friendly. I have made this line number color lighter so that it is easier for the user to notice.
	~	I have changed the line number color of diff viewer for better user visibility. The color in diff viewer before was black by default and was a distraction to the actual code. I have made this line number color lighter and less noticable so that the code is easier to read.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (d397b2d)

Ship it!

```
Ship It!
```