Closed Bug 1045751 Opened 6 years ago Closed 6 years ago

Shrink tile (pin/block/gear) button sizes by a little bit

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: Mardak, Assigned: cheekujodhpur, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 5 obsolete files)

From phlsa:  The buttons are very large relative to the size of the tile. Given that the 99.9% case for the user is *not* to click on those buttons, we should consider shrinking them. I tried quickly scaling them to 70% which works quite well (http://cl.ly/image/2h2P2G103938).

The current icons are 32x32, so 70% would be 22.4. What size should it be? 24? 22? These are SVG, so we can scale it to whatever we want.
Points: --- → 1
Blocks: 1036284
Summary: Shrink tile (pin/block) button sizes by a little bit → Shrink tile (pin/block/gear) button sizes by a little bit
Flags: firefox-backlog+
Note to people wanting to take this bug:

The code is scattered across browser/themes/shared/newtab (CSS and SVGs) and browser/base/content/newtab (JavaScript and XML source). Let me know if you need more help, either in a comment here or on IRC.
Mentor: manishearth
Whiteboard: [good first bug]
Attached patch Suggested patch (obsolete) — Splinter Review
Didn't change gear, debatable, like it the way it is. 

Didn't change the .svg files to keep rescaling easier in future
Attachment #8470591 - Flags: review?(manishearth)
Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

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

LGTM, though are you going to resize the gear in a different patch?

I won't be able to grant review+ for this patch, but someone else can.
Attachment #8470591 - Flags: review?(manishearth)
Attachment #8470591 - Flags: review?(bmcbride)
Attachment #8470591 - Flags: feedback+
Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

dcrobot says the gear should be 27px and phlsa says tile buttons 22px. Although for the tile buttons, there's a shadow so 24px could be fine and 28px for similar svg spacing?

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> .newtab-control {
>   background-color: transparent;
>   border: none;
>-  height: 32px;
>-  width: 32px;
>+  height: 24px;
>+  width: 24px;
> }
> 
> .newtab-control-pin,
> .newtab-site[pinned] .newtab-control-pin:hover:active {
>   background-image: -moz-image-rect(url(chrome://browser/skin/newtab/controls.svg), 0, 96, 32, 64);
>+  background-size:24px;
nit: space after the ":"

Also, could probably move the background-size to the .newtab-control and explicitly background-size: auto for sponsored although that icon be removed soon with bug 1040369.
Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

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

What Ed said in comment 5, including moving background-size.

And apparently we want the gear at 27px?
Attachment #8470591 - Flags: review?(bmcbride) → review-
I agree that the pin should have been smaller. I made it 24 px specifically after the older versions which were pretty fine. 

One of the reasons 'pin' looks better when smaller is that it obstructs the background less. That is not true in case of the gear. My opinion is that the gear should be left as it is.

Whether we want the pin to 22/24/26 is still open to discussion.
Ok, over to UX then.
Flags: needinfo?(athornburgh)
24 pixels is fine with me.
Flags: needinfo?(athornburgh)
Assignee: nobody → cheekujodhpur
Status: NEW → ASSIGNED
(In reply to Aaron from comment #9)
> 24 pixels is fine with me.

What about the gear icon?
Flags: needinfo?(athornburgh)
The gear icon should be 28 x 28
Flags: needinfo?(athornburgh)
Kumar, you can create the patch ignoring newtab-control-sponsored as I'm removing it in bug 1040369. And dcrobot says the gear should be 28px.
Attachment #8470591 - Attachment is obsolete: true
Attachment #8474042 - Flags: review?(adw)
Attachment #8474042 - Flags: feedback?(manishearth)
Comment on attachment 8474042 [details] [diff] [review]
Version#2, pin/block to 24px, gear to 28px

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> #newtab-customize-button {
>   background-color: transparent;
>   background-image: -moz-image-rect(url(chrome://browser/skin/newtab/controls.svg), 0, 32, 32, 0);
>+  background-size:28px auto;
Is the auto necessary? You don't have it below for the 24px controls and it seems to work. Also nit: space after the ":".

> .newtab-control {
>   background-color: transparent;
You should be able to set the background-size here...

> .newtab-control-sponsored {
>-  width: 24px;
>-  height: 24px;
>+  width: 32px;
>+  height: 32px;
>   background-image: url(chrome://browser/skin/newtab/controls.png);
>   background-position: -249px -1px;
... then override it here.. I think?
Attached file fix_1045751.patch (obsolete) —
Made suggested changes to 8474042 (Version#2)
Attachment #8474042 - Attachment is obsolete: true
Attachment #8474042 - Flags: review?(adw)
Attachment #8474042 - Flags: feedback?(manishearth)
Attachment #8474071 - Flags: review?(adw)
Attachment #8474071 - Flags: feedback?(manishearth)
Attachment #8474071 - Attachment is obsolete: true
Attachment #8474071 - Attachment is patch: false
Attachment #8474071 - Flags: review?(adw)
Attachment #8474071 - Flags: feedback?(manishearth)
Attachment #8474073 - Flags: review?(adw)
Attachment #8474073 - Flags: feedback?(manishearth)
Comment on attachment 8474073 [details] [diff] [review]
Version#3: Made suggested changes to Version#2

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> .newtab-control-sponsored {
>   background-image: url(chrome://browser/skin/newtab/controls.png);
>+  background-size: 32px;
> }
> 
> @media (min-resolution: 2dppx) {
>   .newtab-control-sponsored {
>     background-image: url(chrome://browser/skin/newtab/controls@2x.png);
>     background-size: 296px;
>   }
> }
Sorry I wasn't more explicit in my previous comment: "... then override it here," but the sponsored control isn't using the svg file, so the background size needs to be of the original controls.png height/width instead of being overridden by the newly added "background-size: 24px;"

You can test if the sponsored icon is showing up correctly by creating a new profile.
Here's what it looks like with background-size: 32px with controls.png for the sponsored button.
Attached patch v4 (obsolete) — Splinter Review
in .newtab-control-sponsored, setting background-size to auto does the job. In fact, I tried all other combinations. This is the only option that works.
Attachment #8474073 - Attachment is obsolete: true
Attachment #8474073 - Flags: review?(adw)
Attachment #8474073 - Flags: feedback?(manishearth)
Also, the lines 200/201 i.e. width and height setting in .newtab-control-sponsored is overridden by another css property to be 14px. Should I remove it altogether?
Comment on attachment 8474073 [details] [diff] [review]
Version#3: Made suggested changes to Version#2

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

LGTM
Attachment #8474073 - Attachment is obsolete: false
(In reply to Kumar Ayush from comment #19)
> Created attachment 8474087 [details] [diff] [review]
> in .newtab-control-sponsored, setting background-size to auto does the job.
> In fact, I tried all other combinations. This is the only option that works.
Thanks for trying out the different combinations.

> Also, the lines 200/201 i.e. width and height setting in
> .newtab-control-sponsored is overridden by another css property to be 14px.
> Should I remove it altogether?
Nice find! Yeah, go ahead and clean that up.
Attachment #8474087 - Attachment description: fix_1045751.patch → v4
Attached patch v5, all done.Splinter Review
Attachment #8474073 - Attachment is obsolete: true
Attachment #8474087 - Attachment is obsolete: true
Comment on attachment 8474107 [details] [diff] [review]
v5, all done.

Thanks Kumar! Looks good to me.
Attachment #8474107 - Flags: review?(adw)
Attachment #8474107 - Flags: feedback+
Attachment #8474087 - Flags: review?(bmcbride)
Comment on attachment 8474087 [details] [diff] [review]
v4

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

Oops
Attachment #8474087 - Flags: review?(bmcbride)
Just in case someone was thinking of prepping Kumar's patch for landing once it gets reviewed, I have it ready with commit author "Kumar Ayush <cheekujodhpur@gmail.com>" etc.
Comment on attachment 8474107 [details] [diff] [review]
v5, all done.

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

Thanks Kumar!

Ed, are you going to land this then?
Attachment #8474107 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/dd0e4667d948
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Blocks: 1057602
Uplift has been managed in bug 1057602
Iteration: --- → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.