1079183 - The new refresh buttons looks a bit small

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Maybe it looks small because the tabs button looks busier/larger. I wonder if the toolbar would look a bit more balanced if new refresh button was a bit larger. Anthony, thoughts?

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

This really depends on how it feels "in hand" but from what I remember (if I did all my math correctly), there was extra space around the icons to allow for more breathing room in an already cramped toolbar area. Especially as a first iteration before we home in on what animation possibilities we could work with, how many buttons we want to add, image/icon sharing to save build size, etc..

But, I'd be happy to revisit this more extensively. Possibly after V1? Don't have my devices on my ATM. heh

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ have you maybe got a screen shot of this? maybe there is actually some incorrect math going on

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot of the url bar

The reload button is 18x18dp, and the tabs panel button appears to be 24x24dp - maybe this is why?

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #3)
> Created attachment 8501427 [details]
> Screenshot of the url bar
> 
> The reload button is 18x18dp, and the tabs panel button appears to be
> 24x24dp - maybe this is why?

Yeah, I'd experiment changing the refresh button dimensions to be about 20x20 or 22x22 to see if it looks better.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_tabs_parts2.zip

Yup, must've adjusted the size without realizing when I made it into those two parts for the "magic". Now it's bigger than the refresh button (which was the right size).

I suggest we adjust the size of this "2 part tabs icon" to achieve an overall balance and then revisit to see if this family of 3 is too small? My hunch is that it'll look a bit different when all 3 match up but we'll see.

I've had to adjust these with some more magic since there was originally some padding that I had to already adjust for. But, let's see if these work! This is a finicky one..

If this works, we will have to update the assets in the toolbar as a part of those larger bugs too.. 

Mike, Ni-ing you cause I remember us spending some time noodling over this icon..

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot, 18dp assets

With no other changes and the tabs counter button appears to be offset, however...

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot, 18dp assets, layout bounds

...the asset appears to actually be centered.

I'm guessing this is an illusion given the layering of the two images and because I previously saw comments about offsetting the asset (but then didn't recreate them for the new tablet because it seemed fine at the time).

Lucas, any idea what to do here?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Swap new tablet tabs counter assets with smaller assets

The associated patch.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot, 18dp assets, off-balance margins

I changed the top margin to 19.5dp and the bottom margin to 16.5dp here, in attempts to eyeball center. Reload looks centered while the menu looks off. :\

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I noticed that the new assets are still 24x24dp, but contain whitespace on the edges that presumably make the desired 18x18dp. I wonder if this could be causing any issues...

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #10)
> I noticed that the new assets are still 24x24dp, but contain whitespace on
> the edges that presumably make the desired 18x18dp. I wonder if this could
> be causing any issues...

Yeah, I'd avoid having padding in the assets. FWIW, I think this is looking a bit too small to me. Maybe 18dp is  the way to go there. Also, how does it look when you have 10+ tabs?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 11 tabs open

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Also, how does it look when you have 10+ tabs?

Ridiculous. I suppose we can decrease the font size though.

Anthony, what say you?

Comment 13

[Lucas Rocha (:lucasr)]

To be honest, I think this is purely a visual perception thing. Something tells me that increasing the refresh icon by 1-2dp will do the trick. Shrinking the tab counter doesn't seem to be right answer here.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah - this 2 part tabs icon is proving to be a beast. FWIW I think you guys have the right idea - it's way too small there. I made a guess at the math but doesn't look like it's working out.

Will have a more extensive look at it tomorrow in the office. Sorry guys! :\

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_refresh.zip

(In reply to Michael Comella (:mcomella) from comment #10)
> I noticed that the new assets are still 24x24dp, but contain whitespace on
> the edges that presumably make the desired 18x18dp. I wonder if this could
> be causing any issues...

I added the same padding as the original 2 part icon, which I think is why this is turning out to be such a problem. I think we need to get rid of these paddings that are "in the asset" they cause too many problems...

In the mean time

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: New reload asset

Anthony, I'm a bit confused: the assets in comment 15 appear to be roughly the same size as the assets we already have (18x18 current vs. 18x19 in mdpi) and according to the diff, only two of the assets are different from their previous iterations (mdpi & xhdpi). Do you know what might be going on here?

That being said, this reload button does appear slightly larger, but it's making me wonder if I have an alignment problem.

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8505856 [details]
New reload asset

yeah you're right about the sizes

I realized this too and that's why I was checking what device you were testing on. It seems that there was a weird rounding thing that happened just with the XHDPI asset.

Lucas - wanted to get your eyes on this. How's that look? (I've been looking at this for too long I think)

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #16)
> That being said, this reload button does appear slightly larger, but it's
> making me wonder if I have an alignment problem.

Just to be clear, i don't think it was you :) I blame myself heh :P

Comment 19

[Lucas Rocha (:lucasr)]

Looking better. Ship it :-)

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, the inconsistency in the sizes and diffs of the images from comment 16 concerns me - do you mind double-checking that the assets you gave me for all sizes are correct?

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey Michael,

Yep, I checked, and I know what's going on here. 

Since these icons were directly traced from the desktop side into vectors early on, they're JUST 1px or 2px off. Basically the math isn't as clean (our newest icons are better). So when resizing/ prepping assets, I picked the side of the icon that generated the sharpest icons/ cleanest math at each screen density. So to answer Lucas' keen eyes, I resized these new ones based on the other side of the icon, thus generating a diff size icon.

In short, the math is ugly, it was an asset issue :) and we should clean those up soon.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Update reload assets to make reload larger

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #21)
> In short, the math is ugly, it was an asset issue :) and we should clean
> those up soon.

Filed bug 1085406.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And bug 1085487 for the alignment issue, as talked about in the tablet meeting.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8507922 [details] [diff] [review]
Update reload assets to make reload larger

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

Yep.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Update reload assets to make reload larger

Ran trimage.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/87d496ae10b4

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87d496ae10b4