Closed Bug 1210862 Opened 9 years ago Closed 9 years ago

Consolidate corner radius in dimens

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files)

MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally
40 bytes, text/x-review-board-request
ally
: review+
Details
MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally
40 bytes, text/x-review-board-request
ally
: review+
Details
MozReview Request: Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally
40 bytes, text/x-review-board-request
ally
: review+
Details 
We have a few different corner sizes [1]. button_corner_radius seems to have been our attempt to consolidate but it's 3dp, not 4dp like I thought was our standard.

Anthony, is there a standard corner radius? Are there any exceptions to that?

If so, let's fix it!

[1]: https://mxr.mozilla.org/mozilla-central/search?string=radius&find=dimens.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Flags: needinfo?(alam)
It's a bit hard for me to understand what visual element these lines of code refer to specifically but from my latest designs, 4dp should be the standard. 

And that's where I'll be starting my designs from unless there's a compelling reason to deviate.

From that list, could you tell me what the follow are?

 - "button_corner_radius" 3dp
 - "tablet_browser_toolbar_menu_item_corner_radius" 5dp
 - "toast_button_corner_radius" 2dp
 - "fxaccount_corner_radius" 3dp
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(In reply to Anthony Lam (:antlam) from comment #1)
> From that list, could you tell me what the follow are?

It would take a long time to create a comprehensive list but...

>  - "button_corner_radius" 3dp

https://mxr.mozilla.org/mozilla-central/search?string=[^_]button_corner_radius&regexp=on&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Looks like the buttons in the share overlay, the setup button in the remote tabs page, and the zoomed view.

>  - "tablet_browser_toolbar_menu_item_corner_radius" 5dp

https://mxr.mozilla.org/mozilla-central/search?string=tablet_browser_toolbar_menu_item_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

The pressed area on the tablet toolbar/tab strip buttons.

>  - "toast_button_corner_radius" 2dp

https://mxr.mozilla.org/mozilla-central/search?string=toast_button_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Toasts.

>  - "fxaccount_corner_radius" 3dp

https://mxr.mozilla.org/mozilla-central/search?string=fxaccount_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Everything round in fxaccount.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #2)
> (In reply to Anthony Lam (:antlam) from comment #1)
> > From that list, could you tell me what the follow are?
> 
> It would take a long time to create a comprehensive list but...

Thanks! this really helps. 

> >  - "button_corner_radius" 3dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=[^_]button_corner_radius&regexp=on&find=mobile%2Fandroid&findi=
> &filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> Looks like the buttons in the share overlay, the setup button in the remote
> tabs page, and the zoomed view.

We could probably unify and update these to be 4dp everywhere

> >  - "tablet_browser_toolbar_menu_item_corner_radius" 5dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=tablet_browser_toolbar_menu_item_corner_radius&find=mobile%2Fan
> droid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> The pressed area on the tablet toolbar/tab strip buttons.

As above, probably best we update to the 4dp. 

> >  - "toast_button_corner_radius" 2dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=toast_button_corner_radius&find=mobile%2Fandroid&findi=&filter=
> ^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> Toasts.

Could you elaborate a bit more here? Our toasts have fully rounded sides so I'm not sure where "2dp" is coming into play.

> >  - "fxaccount_corner_radius" 3dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=fxaccount_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^
> \0]*%24&hitlimit=&tree=mozilla-central
> 
> Everything round in fxaccount.

This one's a bit more finicky. There's more moving parts here so maybe we can leave this for now and avoid causing some chaos.

All in all, it might be best if we can get on Vidyo. These mostly sound straight forward from a UI stand point. :) Let's ping on IRC
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
I'll throw up a build for the first two.

(In reply to Anthony Lam (:antlam) from comment #3)
> Could you elaborate a bit more here? Our toasts have fully rounded sides so
> I'm not sure where "2dp" is coming into play.

It's apparently 2dp on GB & 24dp on KitKat+ – I don't have a pre-KK device to test on now.

> > Everything round in fxaccount.
> 
> This one's a bit more finicky. There's more moving parts here so maybe we
> can leave this for now and avoid causing some chaos.

Which moving parts?
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
$: ag "android:radius=\"[0-9]"
drawable/search_suggestion_button.xml
9:            <corners android:radius="4dp"/>
...

drawable/tab_thumbnail.xml
14:            <corners android:radius="3dp"/>
...

Search suggestions have a radius of 4dp and tab thumbnails have a radius of 3dp – should we update these too?
Assignee: nobody → michael.l.comella
Build w/ tablet_browser_toolbar_menu_item_corner_radius & button_corner_radius = 4dp:
  https://people.mozilla.com/~mcomella/apks/mcomella-1210862_01.apk
(In reply to Michael Comella (:mcomella) from comment #4)
> > > Everything round in fxaccount.
> > 
> > This one's a bit more finicky. There's more moving parts here so maybe we
> > can leave this for now and avoid causing some chaos.
> 
> Which moving parts?

I'm just not confident the UI is exactly as I left it since the original design and implementation. I'm pretty sure it's been touched by others since so I'm hesitant to say we should just change this one.


(In reply to Michael Comella (:mcomella) from comment #5)
> $: ag "android:radius=\"[0-9]"
> drawable/search_suggestion_button.xml
> 9:            <corners android:radius="4dp"/>
> ...
> 
> drawable/tab_thumbnail.xml
> 14:            <corners android:radius="3dp"/>
> ...
> 
> Search suggestions have a radius of 4dp and tab thumbnails have a radius of
> 3dp – should we update these too?

WFM - let's do it
Flags: needinfo?(alam)
Comment on attachment 8670860 [details]
MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally
Attachment #8670860 - Flags: review?(ally)
Comment on attachment 8670861 [details]
MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally
Comment on attachment 8670860 [details]
MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

https://reviewboard.mozilla.org/r/21465/#review19437

One of those bugs where the hard work all comes before the patch. :)
Attachment #8670860 - Flags: review?(ally) → review+
Comment on attachment 8670861 [details]
MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

https://reviewboard.mozilla.org/r/21467/#review19439

Thanks!

::: mobile/android/base/resources/values/dimens.xml
(Diff revision 2)
> -    <dimen name="tablet_browser_toolbar_menu_item_corner_radius">5dp</dimen>

I suspect I might be grateful you've done this work when I start Big Button Mode.
Attachment #8670861 - Flags: review?(ally) → review+
Comment on attachment 8671540 [details]
MozReview Request: Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally

https://reviewboard.mozilla.org/r/21585/#review19441

::: mobile/android/base/resources/drawable/tab_thumbnail.xml:14
(Diff revision 1)
> -            <corners android:radius="3dp"/>            
> +            <corners android:radius="@dimen/standard_corner_radius"/>

thanks for getting rid of that whitespace while you're here

Thanks!
Attachment #8671540 - Flags: review?(ally) → review+
https://hg.mozilla.org/integration/fx-team/rev/d53aaea8928228b4c86bfd2b76a34ae96963bdd7
Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

https://hg.mozilla.org/integration/fx-team/rev/92ae0a8c5183ec01d9afd3ebb62e77e2dd57fdf2
Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

https://hg.mozilla.org/integration/fx-team/rev/e82e79d46b917ccb8a4083130c4ee9c21d85ca2f
Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: