1232447 - Increase FxA item height to be same as other items in Settings

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Screenshot_20151214-132838.png

I noticed this in my settings - today's Nightly, Nexus 5X.

All items in this list should be the same height. This makes the FxA button look less like a "button" and its harder to press. Visually, it's also very unpolished looking.

Comment 1

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

Vivek, is this an easy fix? :)

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8698179 [details]
Screenshot_20151214-132838.png

My guess is that we need to explicitly handle the case when the "subtitle" is empty.

antlam, we always have an email address, but we don't always have a "display name".  When we do have a display name, we should show

Anthony Lam
[e-mail]

Perhaps we should show something like

Signed in as
[e-mail]

to keep the spacing when we don't have a display name?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Are you sure we show the display name in settings? It does not show mine but I can see it in the status activity.

Comment 4

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

(In reply to Nick Alexander :nalexander from comment #2)
> Comment on attachment 8698179 [details]
> Screenshot_20151214-132838.png
> 
> My guess is that we need to explicitly handle the case when the "subtitle"
> is empty.
> 
> antlam, we always have an email address, but we don't always have a "display
> name".  When we do have a display name, we should show
> 
> Anthony Lam
> [e-mail]
> 
> Perhaps we should show something like
> 
> Signed in as
> [e-mail]
> 
> to keep the spacing when we don't have a display name?

Are we not able to center the item without a second "subtitle" line?

If not, maybe something like "Last synced" or "Manage account" would be more helpful?

[e-mail]
0 minutes ago

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1232447 : Update Sync now preference setting item height r?nalexander

* SyncPreference title summary, title and icon update handled from Fragment instead of the view.

Review commit: https://reviewboard.mozilla.org/r/30341/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30341/

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Vivek Balakrishnan[:vivek] from comment #5)
> Created attachment 8706470 [details]
> MozReview Request: Bug 1232447 : Update Sync now preference setting item
> height r?nalexander
> 
> * SyncPreference title summary, title and icon update handled from Fragment
> instead of the view.
> 
> Review commit: https://reviewboard.mozilla.org/r/30341/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/30341/

vivek -- why is it necessary to fold the logic into the fragment?

Comment 7

[Vivek Balakrishnan[:vivek]]

@Nick: I think it is better to have the account processing logic in Fragment rather than in the Preference. Further, this way we can also tie in with Fragment callbacks such as to cancel the pending request for the target. Please correct me if I am wrong

Comment 8

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8706470 [details]
MozReview Request: Bug 1232447 : Update Sync now preference setting item height r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30341/diff/1-2/

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8706470 [details]
MozReview Request: Bug 1232447 : Update Sync now preference setting item height r?nalexander

https://reviewboard.mozilla.org/r/30341/#review28717

I gave this a quick skim.  I'm fine with this, although I still don't understand why it's better to do this in the fragment directly rather than delegate to the pref.  But if it addresses the issue, roll on.

Sorry this took so long.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java:121
(Diff revisions 1 - 2)
> +            handler = new Handler();

When would `syncPreference` be null?  Prefer to crash in this situation, since it means something has gone wrong with the XML prefs.

Comment 10

[Vivek Balakrishnan[:vivek]]

https://reviewboard.mozilla.org/r/30341/#review28717

> When would `syncPreference` be null?  Prefer to crash in this situation, since it means something has gone wrong with the XML prefs.

GeckoPreferenceFragment in tablets can load xml layouts that does not have SyncPreference [1]. 

[1]  https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java#74

Comment 11

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.