Page MenuHomePhabricator

New color scheme and text display for diffs
Closed, ResolvedPublic

Description

Several commits have been done to change the appearence of diffs to accomodate people with color blindness. However, some changes were not discussed. That is why I submit a new proposal that encompasses several changes detailed below.

Color changes:

  • The lines in td.diff-deletedline is changed to have a soft yellow background, with the removed text having an amber background and black text in bold.
  • The lines in td.diff-addedline is changed to have a light blue background, with the added text having a lavender background and black text in bold.
  • The lines in td.diff-context will have a slightly lighter grey background.

Much consideration has been applied to make sure all used colors match in hue and brightness levels.

Text display changes:

  • Instead of only the changed text being displayed with white-space:pre-wrap, the entire line in a cell is. This will show indents and any removed/added spaces, which greatly helps in code review.
  • The font-size will be changed from 'smaller' to 88%, which will ensure a proper, legible font-size accross browsers.

Screenshot available at File:ProposedDiffChanges.png

To test the new color scheme, put the following in your CSS:

/* Diffs */
td.diff-context,
td.diff-addedline,
td.diff-deletedline {
    font-size: 88%;
    white-space: pre-wrap;
}
td.diff-context {
    background: #F2F2F2;
}
td.diff-addedline {
    background: #E0ECFF;
}
td.diff-deletedline {
    background: #FCF8CC;
}
td.diff-addedline .diffchange {
    background: #B0C8FF;
    color: #000;
}
td.diff-deletedline .diffchange {
    background: #FFD084;
    color: #000;
}

Version: unspecified
Severity: enhancement

Details

Reference
bz33335

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:06 AM
bzimport set Reference to bz33335.
bzimport added a subscriber: Unknown Object (MLST).
Edokter created this task.Dec 22 2011, 7:49 PM

Created attachment 9758
Patch for mediawiki.action.history.diff.css

Attached:

Adding self to CC to track this bug. Not going to poke it before january though.

Applied in r107127. Erwin, thanks for the contribution, and taking Brandon's considerations into account.

Created attachment 9855
Patch for mediawiki.action.history.diff.css - fix missing line

In my previous patch, I have accidentally left out one line. Can this still be added?

Attached:

I apparently forgot to reopen...

Does that vertical-align: top; serves any specific purpose? Looks like the text already take all the available cell height.

There are situations where one side has a lot of text, while the other side only has one line. That line would dangle in the middle of nowhere instead of on the top of the cell.

Sample diff on enwiki: http://en.wikipedia.org/w/index.php?title=Astrid_Peth&diff=472506437&oldid=472503981

Note that the vertical-align:top has already been implemented locally on enwiki, so you see the intended effect.

Makes perfect sense. I have applied the change with r109932. Thanks!

Diff style reverted back to status que as of 1.18.0 in r112750.

Bug is still valid and needs a solution, see r112750 commit-msg for details

  • Bug 33139 has been marked as a duplicate of this bug. ***
  • Bug 11374 has been marked as a duplicate of this bug. ***
  • This bug has been marked as a duplicate of bug 11374 ***
Prtksxna updated the task description. (Show Details)May 24 2017, 11:34 AM
hashar removed a subscriber: hashar.May 24 2017, 12:27 PM