Closed
Bug 1361695
Opened 7 years ago
Closed 7 years ago
Add illustration to about:license
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: dao, Assigned: Towkir)
References
(Blocks 1 open bug)
Details
(Whiteboard: [reserve-photon-visual][p4])
Attachments
(2 files, 3 obsolete files)
6.25 KB,
image/svg+xml
|
Details | |
9.83 KB,
patch
|
Towkir
:
review+
|
Details | Diff | Splinter Review |
mockup: https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229803192 image: attachment 8861726 [details]
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P2
Updated•7 years ago
|
Whiteboard: [photon-visual][p2] → [reserve-photon-visual][p2]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Reporter | ||
Updated•7 years ago
|
Whiteboard: [reserve-photon-visual][p2] → [reserve-photon-visual][p4]
Assignee | ||
Comment 2•7 years ago
|
||
I will submit a patch soon, assigning for now
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
Looks good on my local build, hope this works :)
Attachment #8897377 -
Flags: review?(dao+bmo)
Updated•7 years ago
|
Priority: P3 → P1
Reporter | ||
Comment 4•7 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-
Comment 5•7 years ago
|
||
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•7 years ago
|
||
Please use this image for the illustration
Assignee | ||
Comment 7•7 years ago
|
||
Hope this is close to what you expected :)
Attachment #8897377 -
Attachment is obsolete: true
Attachment #8911410 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 8•7 years ago
|
||
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•7 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•7 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•7 years ago
|
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 11•7 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•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2b5c9dd4f59
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Reporter | ||
Comment 15•7 years ago
|
||
I think we should let this ride release trains and not bother uplifting it to 57.
status-firefox57:
--- → wontfix
Comment 16•7 years ago
|
||
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.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•