Add illustration to about:license

VERIFIED FIXED in Firefox 58

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: dao, Assigned: Towkir)

Tracking

(Blocks 1 bug)

Trunk
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 verified)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

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
Reporter

Updated

2 years ago
Whiteboard: [reserve-photon-visual][p2] → [reserve-photon-visual][p4]
Assignee

Comment 2

2 years ago
I will submit a patch soon, assigning for now
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Assignee

Comment 3

2 years ago
Posted 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
Reporter

Comment 4

2 years ago
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
Posted image about-license.svg
Please use this image for the illustration
Assignee

Comment 7

2 years ago
Posted 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)
Assignee

Comment 8

2 years ago
Posted 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)
Reporter

Comment 9

2 years ago
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+
Assignee

Comment 10

2 years ago
(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 ?
Assignee

Updated

2 years ago
Flags: needinfo?(dao+bmo)
Reporter

Comment 11

2 years ago
(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)
Assignee

Comment 12

2 years ago
All right, 
Please add checkin-needed if this is a go.
Attachment #8911413 - Attachment is obsolete: true
Attachment #8912714 - Flags: review+

Comment 13

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b5c9dd4f59
Added illustration to about:license; r=dao

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2b5c9dd4f59
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Reporter

Comment 15

2 years ago
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+

Updated

2 months ago
Regressions: 1543143
You need to log in before you can comment on or make changes to this bug.