Closed Bug 1361695 Opened 7 years ago Closed 7 years ago

Add illustration to about:license

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: dao, Assigned: Towkir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual][p4])

Attachments

(2 files, 3 obsolete files)

Flags: qe-verify?
Priority: -- → P2
Putting on reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-visual][p2] → [reserve-photon-visual][p2]
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Whiteboard: [reserve-photon-visual][p2] → [reserve-photon-visual][p4]
I will submit a patch soon, assigning for now
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attached patch license-illustration.patch (obsolete) — Splinter Review
Looks good on my local build, hope this works :)
Attachment #8897377 - Flags: review?(dao+bmo)
Priority: P3 → P1
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.
Attachment #8897377 - Flags: review?(dao+bmo) → review-
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
Attached image about-license.svg
Please use this image for the illustration
Attached patch license-illustration.patch (obsolete) — Splinter Review
Hope this is close to what you expected :)
Attachment #8897377 - Attachment is obsolete: true
Attachment #8911410 - Flags: review?(dao+bmo)
Attached patch license-illustration.patch (obsolete) — Splinter Review
Sorry, a bit of changes were missed on the previous one, ignore that.
Review this one.
Attachment #8911410 - Attachment is obsolete: true
Attachment #8911410 - Flags: review?(dao+bmo)
Attachment #8911413 - Flags: review?(dao+bmo)
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!
Attachment #8911413 - Flags: review?(dao+bmo) → review+
(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 ?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
All right, 
Please add checkin-needed if this is a go.
Attachment #8911413 - Attachment is obsolete: true
Attachment #8912714 - Flags: review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b5c9dd4f59
Added illustration to about:license; r=dao
https://hg.mozilla.org/mozilla-central/rev/f2b5c9dd4f59
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I think we should let this ride release trains and not bother uplifting it to 57.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1543143
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: