1361695 - Add illustration to about:license

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Dão Gottwald [:dao]]

mockup: https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229803192

image: attachment 8861726 [details]

Comment 1

[Justin Dolske [:Dolske]]

Putting on reserve backlog for now.

Comment 2

[[:Towkir] Ahmed]

I will submit a patch soon, assigning for now

Comment 3

[[:Towkir] Ahmed]

Attachment: license-illustration.patch

Looks good on my local build, hope this works :)

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8897377 [details] [diff] [review]
license-illustration.patch

Sorry it took me so long to get to this review. This patch is a good start, but not quite there yet.

>-#ifdef APP_LICENSE_BLOCK
>-#includesubst @APP_LICENSE_BLOCK@
>-#endif

>+          #ifdef APP_LICENSE_BLOCK
>+          #includesubst @APP_LICENSE_BLOCK@
>+          #endif

This breaks preprocessing. These lines must not have any spaces before #.

>--- a/toolkit/themes/shared/in-content/info-pages.inc.css
>+++ b/toolkit/themes/shared/in-content/info-pages.inc.css

>+/* License Illustration */
>+.license-header {
>+  display: flex;
>+  align-items: center;
>+}

Can you move this into toolkit/content/license.html? It's irrelevant to the other pages using info-pages.css.

I also think you should probably use float: inline-end; for the image.

Comment 5

[Erica Wright [:ewright]]

Just a note before this merges. We have decided to go with light blue images, not coloured ones. I'll attach the new image here as soon as I get it. 
cc :shorlander

Comment 6

[Erica Wright [:ewright]]

Attachment: about-license.svg

Please use this image for the illustration

Comment 7

[[:Towkir] Ahmed]

Attachment: license-illustration.patch

Hope this is close to what you expected :)

Comment 8

[[:Towkir] Ahmed]

Attachment: license-illustration.patch

Sorry, a bit of changes were missed on the previous one, ignore that.
Review this one.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8911413 [details] [diff] [review]
license-illustration.patch

>+body[dir="rtl"] .rights-header{
>+   background-position: left center;

nit: please add a space before {, reduce the second line's indentation by one space, and use background-position-x: left; here instead of background-position: left center;

Thanks!

Comment 10

[[:Towkir] Ahmed]

(In reply to Dão Gottwald [::dao] from comment #9)
> and use background-position-x: left; here instead of
> background-position: left center;
should it be like this :
background-position-x: left;
background-position-y: center;

or the -y alignment does not matter ?

Comment 11

[Dão Gottwald [:dao]]

(In reply to [:Towkir] Ahmed from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #9)
> > and use background-position-x: left; here instead of
> > background-position: left center;
> should it be like this :
> background-position-x: left;
> background-position-y: center;
> 
> or the -y alignment does not matter ?

"background-position-y: center" is already implied by the previous rule as part of "background-position: right center", so you shouldn't have to repeat that.

Comment 12

[[:Towkir] Ahmed]

Attachment: license-illustration.patch

All right, 
Please add checkin-needed if this is a go.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b5c9dd4f59
Added illustration to about:license; r=dao

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f2b5c9dd4f59

Comment 15

[Dão Gottwald [:dao]]

I think we should let this ride release trains and not bother uplifting it to 57.

Comment 16

[Valentina Claudia Ona]

I verified this issue using Nightly 58.0a1 on Windows 10 x64, Windows 7 x32, Ubuntu 16.04 and Mac OS X 10.12 with build ID 20171001220301. 
I will mark this as verified fixed.