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

RESOLVED FIXED in Firefox 33

Status

()

Firefox
New Tab Page
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Mardak, Assigned: Kumar Ayush, Mentored)

Tracking

Trunk
Firefox 34
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 1030832, 977787
Points: --- → 1
(Reporter)

Updated

4 years ago
Blocks: 1036284
(Reporter)

Updated

4 years ago
Summary: Shrink tile (pin/block) button sizes by a little bit → Shrink tile (pin/block/gear) button sizes by a little bit

Updated

4 years ago
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]
(Assignee)

Comment 3

4 years ago
Created attachment 8470591 [details] [diff] [review]
Suggested patch

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+
(Reporter)

Comment 5

4 years ago
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-
(Assignee)

Comment 7

4 years ago
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)

Comment 9

4 years ago
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)

Comment 11

4 years ago
The gear icon should be 28 x 28
Flags: needinfo?(athornburgh)
(Reporter)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
Created attachment 8474042 [details] [diff] [review]
Version#2, pin/block to 24px, gear to 28px
Attachment #8470591 - Attachment is obsolete: true
Attachment #8474042 - Flags: review?(adw)
Attachment #8474042 - Flags: feedback?(manishearth)
(Reporter)

Comment 14

4 years ago
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?
(Assignee)

Comment 15

4 years ago
Created attachment 8474071 [details]
fix_1045751.patch

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)
(Assignee)

Updated

4 years ago
Attachment #8474071 - Attachment is obsolete: true
Attachment #8474071 - Attachment is patch: false
Attachment #8474071 - Flags: review?(adw)
Attachment #8474071 - Flags: feedback?(manishearth)
(Assignee)

Comment 16

4 years ago
Created attachment 8474073 [details] [diff] [review]
Version#3: Made suggested changes to Version#2
Attachment #8474073 - Flags: review?(adw)
Attachment #8474073 - Flags: feedback?(manishearth)
(Reporter)

Comment 17

4 years ago
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.
(Reporter)

Comment 18

4 years ago
Created attachment 8474083 [details]
screenshot of v3 (background-size: 32px;)

Here's what it looks like with background-size: 32px with controls.png for the sponsored button.
(Assignee)

Comment 19

4 years ago
Created attachment 8474087 [details] [diff] [review]
v4

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)
(Assignee)

Comment 20

4 years ago
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
(Reporter)

Comment 22

4 years ago
(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.
(Reporter)

Updated

4 years ago
Attachment #8474087 - Attachment description: fix_1045751.patch → v4
(Assignee)

Comment 23

4 years ago
Created attachment 8474107 [details] [diff] [review]
v5, all done.
Attachment #8474073 - Attachment is obsolete: true
Attachment #8474087 - Attachment is obsolete: true
(Reporter)

Comment 24

4 years ago
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)
(Reporter)

Comment 26

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa-]
(Reporter)

Updated

4 years ago
Blocks: 1057602
Uplift has been managed in bug 1057602
status-firefox33: --- → fixed
status-firefox34: --- → fixed
(Reporter)

Updated

4 years ago
status-firefox33: fixed → affected
(Reporter)

Updated

4 years ago
status-firefox33: affected → fixed

Updated

4 years ago
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.