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
TitleReferenceAuthorSource BranchDest Branch
Eliminate use of deprecated 'serialize' term as much as possiblerepos/abstract-wiki/wikifunctions/function-orchestrator!146jforresterT353354main
Update function-schemata sub-module to HEAD (a40fe85)repos/abstract-wiki/wikifunctions/wikilambda-cli!34jforrestersync-function-schematamain
Update function-schemata sub-module to HEAD (a40fe85)repos/abstract-wiki/wikifunctions/function-evaluator!186jforrestersync-function-schematamain
Update function-schemata sub-module to HEAD (a40fe85)repos/abstract-wiki/wikifunctions/function-orchestrator!145jforrestersync-function-schematamain
Start renaming binary-format things to binaryFormatterrepos/abstract-wiki/wikifunctions/function-schemata!102jforresterT353354main
Update function-schemata sub-module to HEAD (97ea4cd)repos/abstract-wiki/wikifunctions/wikilambda-cli!33jforrestersync-function-schematamain
Update function-schemata sub-module to HEAD (97ea4cd)repos/abstract-wiki/wikifunctions/function-orchestrator!142jforrestersync-function-schematamain
Update function-schemata sub-module to HEAD (97ea4cd)repos/abstract-wiki/wikifunctions/function-evaluator!179jforrestersync-function-schematamain
Replace old 'derializer' term with 'type converter'repos/abstract-wiki/wikifunctions/function-evaluator!178jforresterT353354main
serialize: Avoid all use of 'derializer' confusing termrepos/abstract-wiki/wikifunctions/function-schemata!100jforresterT353354main
definitions: Re-label Z46 and Z64 as Type converters for clarityrepos/abstract-wiki/wikifunctions/function-schemata!99jforresterT353354main
Add tools for stopping grid jobs without stopping k8s jobsrepos/cloud/toolforge/disable-tool!2andrewgridstopmaster
Small refactors + additional new scripts for shutting off grid thingsrepos/cloud/toolforge/disable-tool!1andrewmastermaster
Show related patches Customize query in GitLab

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

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 ***