Closed Bug 1182187 Opened 7 years ago Closed 7 years ago

Remove one-off styling for .identity-popup-button

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch identity-popup-button.diff (obsolete) — Splinter Review
See bug 1175239 comment 28. I understand the desire to "update older styles" (although I'll note that there's nothing inherently old about native buttons; they automatically get design updates along with the host OS), but like I said this isn't the right way to implement that.

This should help with bug 1181803 too.
Attachment #8631714 - Flags: review?(ttaubert)
I think it doesn't look too terrible... Aislinn, what do you think?

http://i.imgur.com/nIVM31B.png
http://i.imgur.com/4sB5F9x.png
Flags: needinfo?(agrigas)
Comment on attachment 8631714 [details] [diff] [review]
identity-popup-button.diff

Needs to be updated due to bug 1180841.
Attachment #8631714 - Flags: review?(ttaubert)
Attached patch identity-popup-button.diff (obsolete) — Splinter Review
Attachment #8631714 - Attachment is obsolete: true
Attachment #8632054 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #1)
> I think it doesn't look too terrible... Aislinn, what do you think?
> 
> http://i.imgur.com/nIVM31B.png
> http://i.imgur.com/4sB5F9x.png

Can you increase the button height so theres more white space between the top and bottom of the text?
Flags: needinfo?(agrigas)
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to agrigas from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #1)
> > I think it doesn't look too terrible... Aislinn, what do you think?
> > 
> > http://i.imgur.com/nIVM31B.png
> > http://i.imgur.com/4sB5F9x.png
> 
> Can you increase the button height so theres more white space between the
> top and bottom of the text?

I re-added the 30px min-height
Attachment #8632136 - Flags: review?(ttaubert)
Attachment #8632054 - Attachment is obsolete: true
Attachment #8632054 - Flags: review?(ttaubert)
(In reply to Dão Gottwald [:dao] from comment #5)
> Created attachment 8632136 [details] [diff] [review]
> patch v2
> 
> (In reply to agrigas from comment #4)
> > (In reply to Tim Taubert [:ttaubert] from comment #1)
> > > I think it doesn't look too terrible... Aislinn, what do you think?
> > > 
> > > http://i.imgur.com/nIVM31B.png
> > > http://i.imgur.com/4sB5F9x.png
> > 
> > Can you increase the button height so theres more white space between the
> > top and bottom of the text?
> 
> I re-added the 30px min-height

This styling doesn't look like our buttons in preferences. The outline stroke is too dark and the text padding is still to narrow...
But that's what our buttons look like...

http://i.imgur.com/sohuR8H.png
http://i.imgur.com/o12UI04.png

Not easy to find that style in the main UI nowadays, it's more or less hidden dialogs most people probably don't encounter day-to-day.
To clarify the style we're going forward with for the new design of control center, we are using the About:Preferences button style. Using the style is a deliberate choice to update the design of some menus to fit the new design styling created for other parts of the browser.
(In reply to agrigas from comment #8)
> To clarify the style we're going forward with for the new design of control
> center, we are using the About:Preferences button style. Using the style is
> a deliberate choice to update the design of some menus to fit the new design
> styling created for other parts of the browser.

This was already understood, let's not go round in circles. The problem here is more nuanced; it seems that this design change wasn't well-planned and thus it was executed in a way that doesn't scale and makes the UI inconsistent. It further seems that the control center doesn't actually need a new button design at all but was rather used as a vehicle for that design change. This may make sense for some design work but it doesn't in this case. What should happen is that we get a bug filed dedicated to that design change, and then we can prioritize that and think about the best way to implement it.
Comment on attachment 8632136 [details] [diff] [review]
patch v2

Review of attachment 8632136 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go with that button style, the patch needs some rebasing and account for the third button we have there now.

The min-height is definitely the right thing to do but it destroys the rounded corners for reasons I don't understand. It makes the button rectangular and puts a weird gradient as the background.
Attachment #8632136 - Flags: review?(ttaubert) → review-
(In reply to Tim Taubert [:ttaubert] from comment #10)
> The min-height is definitely the right thing to do but it destroys the
> rounded corners for reasons I don't understand. It makes the button
> rectangular and puts a weird gradient as the background.

This sounds super weird. Is this on OS X? I'll try adding padding on .button-box instead.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8632136 - Attachment is obsolete: true
Attachment #8637271 - Flags: review?(ttaubert)
Comment on attachment 8637271 [details] [diff] [review]
patch v3

This looks better, thanks. The black border makes buttons very heavy though, which doesn't quite work with the light lines in the rest of the panel. Can we make it the same as for the bookmarks edit panel?

http://i.imgur.com/gpzDgUd.png
Attached patch patch v4Splinter Review
Attachment #8637271 - Attachment is obsolete: true
Attachment #8637271 - Flags: review?(ttaubert)
Attachment #8637295 - Flags: review?(ttaubert)
Comment on attachment 8637295 [details] [diff] [review]
patch v4

Review of attachment 8637295 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful. Thank you very much for helping us get it right!
Attachment #8637295 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/7cbcece973de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.