Closed Bug 1064461 Opened 5 years ago Closed 5 years ago

Clean up snippet visuals

Categories

(Firefox for Android :: Theme and Visual Design, defect)

x86
Android
defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: antlam, Unassigned)

References

Details

Attachments

(3 files)

Our snippet's UI could use a little bit of a clean up in the structure to be more consistent.

Simple hit area/ icon area stuff. Will post a mock up with specs later. Attaching example snippet for the time being.

Seems like a quick win.
Attached image prev_snippetspec.png
Attaching spec!
I would like to take this bug. 
Can you give a brief summary on what exactly needs to be done in this?
Flags: needinfo?(alam)
This looks like a more overall clean up of the visuals like copy and padding, whereas bug 966654 was specifically about address issues of the 'x' size. 

We could split up the work into these two different bugs to help track that way if you'd like.
Flags: needinfo?(alam)
Hi, So how are we going to spilt the work into these two bugs? Can you please elaborate what all changes should I do in both the bugs separately. Thanks :)
Flags: needinfo?(alam)
Attached image spec_snippet_mock2.png
I was thinking we could split up the "close button hit are is too small" into bug 966654 and leave this one for everything else.

Bug 966654: 
 - Enlarge hit area to hit the edges of the snippet

This bug:
 - tweak padding of everything else
 - make sure all the measurements line up

Not sure if this is necessary, going to NI Margaret here to see what she thinks.
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
(In reply to Anthony Lam (:antlam) from comment #5)
> Created attachment 8536478 [details]
> spec_snippet_mock2.png
> 
> I was thinking we could split up the "close button hit are is too small"
> into bug 966654 and leave this one for everything else.
> 
> Bug 966654: 
>  - Enlarge hit area to hit the edges of the snippet
> 
> This bug:
>  - tweak padding of everything else
>  - make sure all the measurements line up
> 
> Not sure if this is necessary, going to NI Margaret here to see what she
> thinks.

Thanks for the mock-up! This clarifies what I've been trying to say in bug 966654, which is that we need the tap area around the close button to extend to the edges of the banner. We can just implement this change over in that bug.

My one piece of feedback on this mock-up is that currently the icon on the left of the banner is 48dp, and I think we should keep that size because this affects the image sizes that the snippets team can use. Can we keep the icon 48dp, but and then reduce the margins around it?
Flags: needinfo?(margaret.leibovic)
Now, what other changes should I do in this bug except the margin changes?
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #7)
> Now, what other changes should I do in this bug except the margin changes?

I think we should wait for a response from antlam about the icon size. But yes, it looks like the only thing left here will be updating the margins around the icon.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #8)
> (In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #7)
> > Now, what other changes should I do in this bug except the margin changes?
> 
> I think we should wait for a response from antlam about the icon size. But
> yes, it looks like the only thing left here will be updating the margins
> around the icon.

Ah, good point. sure - let's try that. 

That would make the margins there go from 18dp to 10dp on each side.
Flags: needinfo?(alam) → needinfo?(jalpreetnanda)
Then, I think we dont need to change anything, as the margins are already 10dp or is there anything else to change ?
Flags: needinfo?(jalpreetnanda) → needinfo?(alam)
Should be good then.
Flags: needinfo?(alam)
So, is there any other change to be done?
Flags: needinfo?(alam)
(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #12)
> So, is there any other change to be done?

Nope, sounds like this bug is solved! Thanks for your help investigating. I'm sorry there wasn't more to do here, but verifying things like this is just as important as writing patches :)
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(alam)
Resolution: --- → WORKSFORME
Happy to help! :)
You need to log in before you can comment on or make changes to this bug.