launchpad-reviewers team mailing list archive

[Peter Makowski <mp+447200@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/canonical/launchpad/icing/style.css b/lib/canonical/launchpad/icing/style.css
> index 125f723..04dd339 100644
> --- a/lib/canonical/launchpad/icing/style.css
> +++ b/lib/canonical/launchpad/icing/style.css
> @@ -663,12 +692,18 @@ table.diff .line-no.active, table.diff .ss-line-no.active {
>    background: url(/@@/add) #f6f6f6 center left no-repeat;
>  }
>  table.diff .diff-chunk, table.diff .diff-file, table.diff .diff-header {
> - font-weight: bold;
> +  background: #f4f6f9;
> +  font-weight: 600;
>  }
> -table.diff .diff-added { background-color: #92ED92; }

It is indeed an accessibility thing, higher contrast makes the text easier to read. 

I'd like to also point out that both colors are almost exactly the same colors that are currently used on GitHub for the background for added and removed lines of code.

Currently removed lines fail WCAG 2.0 accessibility test entirely. After this change text passes a WCAG accessibility check with level AAA (the strictest accessibility check).

Comparison of color contrast before and after this change:

type of line | WCAG 2.0 score before | WCAG 2.0 score after
removed | Failed (ratio 2.80)  | Passed AAA (ratio 2.80)
added | Passed AA (4.99) | Passed AAA (9.25)

(AA is a strict, and AAA the strictest accessibility check)

You can verify the above yourself e.g. using the WEBAIM Contrast Checker: https://webaim.org/resources/contrastchecker/

> -table.diff .diff-removed { background-color: #FF7F7F; }
> +table.diff .diff-added { background-color: #e1ffe7; }
> +table.diff .diff-removed { background-color: #ffe6e4; }
>  table.diff .inline-comments > td > div {
> -  margin: 0 1em 1.5em 1em;
> +  margin: 0;
> +}
> +
> +table.diff .inline-comment {
> +    border-top: 1px solid #ddd;
> +    border-bottom: 1px solid #ddd;
>  }
>  table.diff .inline-comments .yui3-ieditor { padding-right:0!important; }
>  table.diff .inline-comments .yui3-ieditor-multiline .yui3-ieditor-btns
> @@ -676,7 +711,8 @@ table.diff .inline-comments .yui3-ieditor-multiline .yui3-ieditor-btns
>    word-wrap: normal;
>  }
>  table.diff .inline-comments .boardComment {
> -  margin: 1em 0;
> +  margin: 0.5rem;
> +  max-width: 43.75rem; /* 700px */

The reason for the change of size was to make it easier to distinguish and separate visually from other sections of the page. This is very similar to how it's done on GitHub. Example of a GitHub comment: https://github.com/facebook/react/pull/27090/files#r1263928941

I'm happy to revert this change if you feel strongly about this or in case users will be unhappy with this decision.

>  }
>  table.diff .inline-comments .boardCommentBody {
>    word-wrap: break-word;


-- 
https://code.launchpad.net/~petermakowski/launchpad/+git/launchpad/+merge/447200
Your team Launchpad code reviewers is requested to review the proposed merge of ~petermakowski/launchpad:update-diff-view-ui into launchpad:master.