Closed Bug 1148050 Opened 5 years ago Closed 5 years ago

Type Control Visual Improvements

Categories

(Firefox :: General, defect, P3)

38 Branch
x86
All
defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 + fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: mmaslaney, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Can you talk a little bit more about what you want to happen in #2?  (I think I understand #1, #3, and #4…)
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.
Attached patch The first cut at the patch. (obsolete) — Splinter Review
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?
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8584614 - Flags: ui-review?(mmaslaney)
(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.
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-
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)
Looks great. ;)
Flags: needinfo?(mmaslaney)
Attached patch The second cut at the patch. (obsolete) — Splinter Review
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 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+
Attached patch The third cut at the patch. (obsolete) — Splinter Review
(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+
Attachment #8585037 - Flags: ui-review?(mmaslaney) → ui-review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attached patch Rebased patch.Splinter Review
Here you go!
Attachment #8585037 - Attachment is obsolete: true
Attachment #8586175 - Flags: ui-review+
Attachment #8586175 - Flags: review+
(Did I get the flags right, Ryan?)
Keywords: checkin-needed
Target Milestone: Firefox 38 → ---
https://hg.mozilla.org/mozilla-central/rev/049a2ddcc904
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Hi Blake, can you provide a point value.
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Flags: needinfo?(bwinton)
Flags: firefox-backlog+
Blocks: 1132074
Points: --- → 2
Flags: needinfo?(bwinton)
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)
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 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?
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+
Depends on: 1188223
You need to log in before you can comment on or make changes to this bug.