Closed
Bug 1139949
Opened 10 years ago
Closed 10 years ago
Make the small swatch of current theme in the themes button smaller and rounded.
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
VERIFIED
FIXED
Firefox 40
People
(Reporter: bwinton, Assigned: farhaan.bukhsh, Mentored)
References
Details
(Whiteboard: [qx:spec] [lang=css] [good first bug])
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gijs
:
review+
phlsa
:
ui-review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Having the swatch is a nice improvement, but it's a little too big.
https://dl.dropboxusercontent.com/u/2301433/ThemeSwatch.png
And while we're modifying the swatch style, it would be nice to have a 3 pixel border-radius, to match the button. :)
Farhaan, you've worked in this code before; did you want to take a shot at quickly fixing this one?
Reporter | ||
Comment 1•10 years ago
|
||
(Although, it looks like the border-radius doesn't do anything to the image, so that part might not be possible.)
assign it to me Blake , I would like to work on it for sure.
Updated•10 years ago
|
Assignee: nobody → farhaan.bukhsh
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•10 years ago
|
||
Awesome! Do you remember where the css is from the previous swatch bug?
Comment 6•10 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #1)
> (Although, it looks like the border-radius doesn't do anything to the image,
> so that part might not be possible.)
overflow: hidden might work.
Ok i will try and let you know, sorry for not updating this long !!
Comment 8•10 years ago
|
||
[Tracking Requested - why for this release]:
This is pretty visible on customize mode (esp. on retina where the image is also pixelly) and we should fix it before releasing 39.
status-firefox39:
--- → affected
tracking-firefox39:
--- → ?
None of the css property is affecting the swatch, I will be glad if someone can lend me a bit help on this one.
I tried a lot of things link of which i will put up. Help required
Assignee | ||
Comment 10•10 years ago
|
||
I tried a lot of stuff but nothing is affecting that swatch need help to resolve this.
List of things I tried:
https://pastebin.mozilla.org/8826870
Updated•10 years ago
|
Flags: needinfo?(jaws)
Updated•10 years ago
|
Flags: needinfo?(bwinton)
Reporter | ||
Comment 11•10 years ago
|
||
Hey Farhaan,
Jared and I have both tried to get the rounded corners working, and they don't seem to be applying for either of us. As a compromise for this version, can we just make the swatch smaller?
In the previous bug, you changed line 149 of browser/themes/shared/customizableui/customizeMode.inc.css to have:
#customization-lwtheme-button {
padding: 2px 7px;
}
#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
-moz-margin-end: 3px;
height: 22px;
}
I think this will probably do what we're asking for…
Thanks,
Blake.
Flags: needinfo?(bwinton)
Comment 12•10 years ago
|
||
We can probably get the border-radius if we use the clip-path CSS property and an SVG shape. Blake, would you like to look in to this?
Flags: needinfo?(jaws) → needinfo?(bwinton)
Reporter | ||
Comment 13•10 years ago
|
||
I've got a bunch of reading list transition bugs to do first, but if no-one else picks it up before me, I'll take a stab at it. (Also, is clip-path and SVG _really_ the best solution here? Cause that's pretty gross, if it is. ;)
Flags: needinfo?(bwinton)
Comment 14•10 years ago
|
||
MattN and Gijs were talking about an alternative approach here, http://logs.glob.uno/?c=mozilla%23fx-team&s=24+Mar+2015&e=24+Mar+2015&h=clip-path#c199079
Assignee | ||
Comment 15•10 years ago
|
||
I guess this works fine please check and let me know if its good!!
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
if that is what you need then its done in this way!
https://pastebin.mozilla.org/8827223
Reporter | ||
Comment 18•10 years ago
|
||
I think that's better than what we currently have… Philipp, do you think this new smaller size is good enough to ship?
Flags: needinfo?(bwinton) → needinfo?(philipp)
Reporter | ||
Comment 19•10 years ago
|
||
Hey Farhaan, sorry for stealing the bug like this, but I had a bit of time this afternoon, so I did some hacking and got it looking like this: https://dl.dropboxusercontent.com/u/2301433/Firefox/ThemeButton.png
(I'm also redirecting the review request to Gijs, since this was his suggestion, and since Jaws isn't accepting review requests for a little while. ;)
Attachment #8584680 -
Flags: ui-review?(philipp)
Attachment #8584680 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 20•10 years ago
|
||
Hey no worries but how did you do it?
Reporter | ||
Comment 21•10 years ago
|
||
Check the diff! ;)
(Seriously, take a look at the diff, and see which parts make sense, and which don't, and ask me about the ones you don't understand… If your question was more "Where did I get the idea?", then the answer is "From the logs Jaws posted in comment 14." :)
Assignee | ||
Comment 22•10 years ago
|
||
Why the change in javascript part is needed? Now direct me to other bug :P
Reporter | ||
Comment 23•10 years ago
|
||
The change in javascript is needed to set the background-image style on the sub-element, instead of setting the image attribute on the button. (If we set the image attribute on the button, we would get two images, one of which wouldn't be rounded. :)
For the next bug, how do you feel about taking another stab at bug 1102710? Or you could finish off the work NTim started in bug 1111094…
Updated•10 years ago
|
Attachment #8584680 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•10 years ago
|
Flags: needinfo?(jaws)
Comment 24•10 years ago
|
||
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…
I didn't apply the patch, but I assume that it looks like the buttons in bwintons screenshots (https://dl.dropboxusercontent.com/u/2301433/Firefox/ThemeButton.png).
That's a definite improvement!
Flags: needinfo?(philipp)
Attachment #8584680 -
Flags: ui-review?(philipp) → ui-review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [qx] [lang=css] [good first bug] → [qx] [lang=css] [good first bug][fixed-in-fx-team]
Comment 26•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [qx] [lang=css] [good first bug][fixed-in-fx-team] → [qx] [lang=css] [good first bug]
Target Milestone: --- → Firefox 40
Comment 27•10 years ago
|
||
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…
Approval Request Comment
[Feature/regressing bug #]: issue with new feature introduced in bug 1073234
[User impact if declined]: customization mode looks a little uglier on the themes button
[Describe test coverage new/current, TreeHerder]: manual testing, no automated coverage, just a styling change
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8584680 -
Flags: approval-mozilla-aurora?
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…
Approving this for aurora. Round away.
Attachment #8584680 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Whiteboard: [qx] [lang=css] [good first bug] → [qx:spec] [lang=css] [good first bug]
Comment 30•9 years ago
|
||
I have successfully reproduce this bug on Firefox nightly 39.0a1 (2015-03-05)
The bug's fix is verified on latest release 40.0 ( build id : 20150807085045)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
QA Whiteboard: [bugday-20150812]
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•