Closed
Bug 1148050
Opened 9 years ago
Closed 9 years ago
Type Control Visual Improvements
Categories
(Firefox :: General, defect, P3)
Tracking
()
People
(Reporter: mmaslaney, Assigned: bwinton)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
3.38 KB,
patch
|
bwinton
:
review+
bwinton
:
ui-review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Here's a mockup of what the Type Controls should look like, versus what they currently look like: http://invis.io/3D2JR8ZJ5 We should try and clean this up a bit.
Assignee | ||
Comment 1•9 years ago
|
||
Can you talk a little bit more about what you want to happen in #2? (I think I understand #1, #3, and #4…)
Reporter | ||
Comment 2•9 years ago
|
||
Sure, the current implementation has the orange rule right above the gray outline, which is technically not wrong, but creates a bit of a blur effect. I'm wondering if we could extend the orange rule's height just one pixel, so that it covers the gray line.
Assignee | ||
Comment 3•9 years ago
|
||
Okay, let me know what you'ld like cleaned up in this version… :) (Screenshot at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV1.png ) One thing I noticed was that the spec had the + on the left, but the implementation has the + on the right. Is that something we should switch, or was that an intentional change?
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #3) > Created attachment 8584614 [details] [diff] [review] > The first cut at the patch. > > Okay, let me know what you'ld like cleaned up in this version… :) > (Screenshot at > https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV1.png ) • Type samples should roughly be the same size (use San Serif as the size benchmark) • Move Type labels slightly closure to the samples > One thing I noticed was that the spec had the + on the left, but the > implementation has the + on the right. Is that something we should switch, > or was that an intentional change? • Yes, thank you for bringing that up, ignore that part of the spec. We decided to move forward with the - | + format across platforms.
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8584614 [details] [diff] [review] The first cut at the patch. • Type samples should roughly be the same size (use San Serif as the size benchmark) • Move Type labels slightly closure to the samples
Attachment #8584614 -
Flags: ui-review?(mmaslaney) → ui-review-
Assignee | ||
Comment 6•9 years ago
|
||
How do you feel about https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV2.png instead? (It's subtle, but I swear I did what you asked for… ;)
Flags: needinfo?(mmaslaney)
Assignee | ||
Comment 8•9 years ago
|
||
Okay, here's the patch with the updated styles.
Attachment #8584614 -
Attachment is obsolete: true
Attachment #8584867 -
Flags: ui-review?(mmaslaney)
Attachment #8584867 -
Flags: review?(bmcbride)
Comment 9•9 years ago
|
||
Comment on attachment 8584867 [details] [diff] [review] The second cut at the patch. Review of attachment 8584867 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/windows/global/aboutReader.css @@ +22,5 @@ > .dark-button { > color: #eeeeee; > background-color: #333333; > + margin-top: -1px; > + height: 61px !important; This affects both the button and the page content. And !important is codesmell. I'd rather see these additions split out to their own selector and moved to be beside the "#color-scheme-buttons > button" selectors. Having a selector like #color-scheme-buttons > .dark-button" would avoid the need for !important, and make it more obvious what it's doing. @@ +352,5 @@ > } > > #color-scheme-buttons > button { > width: 33.33%; > + border: 0; There's a selector at line 344 that sets top/right/bottom border to 0 for these, which is now redundant. @@ +367,5 @@ > + font-size: 58px; > + height: 100px; > +} > + > +#font-type-buttons > .serif-button { This would benefit from a comment.
Attachment #8584867 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #9) > ::: toolkit/themes/windows/global/aboutReader.css > @@ +22,5 @@ > > .dark-button { > > color: #eeeeee; > > background-color: #333333; > > + margin-top: -1px; > > + height: 61px !important; > > This affects both the button and the page content. And !important is > codesmell. > > I'd rather see these additions split out to their own selector and moved to > be beside the "#color-scheme-buttons > button" selectors. Having a selector > like #color-scheme-buttons > .dark-button" would avoid the need for > !important, and make it more obvious what it's doing. Good call. Changed! > @@ +352,5 @@ > > #color-scheme-buttons > button { > > width: 33.33%; > > + border: 0; > There's a selector at line 344 that sets top/right/bottom border to 0 for > these, which is now redundant. That selector also affected the other buttons, so I've moved this line up to that block, since the border-left gets overridden in the other buttons. > @@ +367,5 @@ > > +#font-type-buttons > .serif-button { > This would benefit from a comment. Added! Thanks for the review! :)
Attachment #8584867 -
Attachment is obsolete: true
Attachment #8584867 -
Flags: ui-review?(mmaslaney)
Attachment #8585037 -
Flags: ui-review?(mmaslaney)
Attachment #8585037 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Attachment #8585037 -
Flags: ui-review?(mmaslaney) → ui-review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
Here you go!
Attachment #8585037 -
Attachment is obsolete: true
Attachment #8586175 -
Flags: ui-review+
Attachment #8586175 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
(Did I get the flags right, Ryan?)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/049a2ddcc904
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/049a2ddcc904
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Comment 16•9 years ago
|
||
Hi Blake, can you provide a point value.
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Flags: needinfo?(bwinton)
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Points: --- → 2
Flags: needinfo?(bwinton)
Comment 17•9 years ago
|
||
Blake, I guess you want this to be uplifted to 38, right? if it is the case, could you fill the uplift request, thanks.
Flags: needinfo?(bwinton)
Assignee | ||
Comment 18•9 years ago
|
||
Hi Sylvestre! I normally leave these to Jaws, who has been doing a lot of work on uplifts for reader-mode/reading-list, but here's the request details… [User impact if declined]: Users will see an uglier font-panel. [Describe test coverage new/current, TBPL]: Manual testing, and has been on mozilla-central for a day. [Risks and why]: Low risk because its a small CSS change impacting only the reader-mode settings popup. [String/UUID change made/needed]: none Do I also need to ask for uplift to 39, or is that implied by tracking-38?
Flags: needinfo?(bwinton)
Comment 19•9 years ago
|
||
Comment on attachment 8586175 [details] [diff] [review] Rebased patch. (I think bwinton meant to flip these flags)
Attachment #8586175 -
Flags: approval-mozilla-beta?
Attachment #8586175 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Comment 20•9 years ago
|
||
Comment on attachment 8586175 [details] [diff] [review] Rebased patch. Should be in 38 beta 2
Attachment #8586175 -
Flags: approval-mozilla-beta?
Attachment #8586175 -
Flags: approval-mozilla-beta+
Attachment #8586175 -
Flags: approval-mozilla-aurora?
Attachment #8586175 -
Flags: approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•