Closed
Bug 1398283
Opened 7 years ago
Closed 7 years ago
General clean-up of Sync Preference screen
Categories
(Firefox for Android Graveyard :: Settings and Preferences, enhancement)
Firefox for Android Graveyard
Settings and Preferences
Tracking
(firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: Grisha, Assigned: Grisha)
References
Details
Attachments
(6 files)
No description provided.
Assignee | ||
Updated•7 years ago
|
Summary: Remove → Remove "signed in as" and "sync: enabled" headers in Sync preferences
Assignee | ||
Comment 1•7 years ago
|
||
"Synced: enabled" seems redundant, or broken, or both. The only way to "disable" sync, other than disconnecting, is to toggle it off under System account preferences, but that doesn't change this header. Perhaps it should? Either way, it's used incorrectly as a header.
"singed in as" header is clearly redundant.
Assignee | ||
Comment 2•7 years ago
|
||
These labels show up as titles of two PreferenceCategories. Removing the title strings leaves large blank spaces where the titles were supposed to be. Perhaps that could be solved with custom styles?
We use these categories as a way to group items, and then to be able to append/remove error states from the defined groups. I think the easier approach is to redefine how we display our error states. I'm thinking that moving error states to the top to a new PreferenceCategory which we toggle on/off, something akin to https://storage.googleapis.com/material-design/publish/material_v_12/assets/0Bzhp5Z4wHba3dEZTUF9idzBHMWc/patterns-errors-userinput19.png will allow us to remove the groupings entirely, and remove these headers.
Assignee | ||
Comment 3•7 years ago
|
||
This is turning into a "cleanup sync pref screen" bug. Changes being made so far:
- re-organize preference grouping, removing two general PreferenceCategories we had prior
- move error states to a separate category, shown at the top as a banner
- move custom auth/sync server url configurations into a new "Additional Settings" category
- remove More...
- remove Manage account
- make tapping on account name/email/avatar lead to "manage account", as opposed to "change avatar" as before
Summary: Remove "signed in as" and "sync: enabled" headers in Sync preferences → General clean-up of Sync Preference screen
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Regular view after cleanup.
Assignee | ||
Comment 5•7 years ago
|
||
Additional Settings section
Assignee | ||
Comment 6•7 years ago
|
||
Error banner
Assignee | ||
Comment 7•7 years ago
|
||
I'm not happy about the margins in Comment 6, but we can address that in Bug 1398288.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1227954#c3 provides some context for current redundancy on Sync Preferences screen (two ways to open about:accounts). Seems that I've settled on something very similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1227954#c4.
Assignee | ||
Comment 11•7 years ago
|
||
More TODO:
- add "Choose what to sync" header above list of data types
- use switches instead of checkboxes
Comment 12•7 years ago
|
||
(In reply to :Grisha Kruglov from comment #11)
> More TODO:
> - add "Choose what to sync" header above list of data types
> - use switches instead of checkboxes
At the time I built this, switches were not the Android standard (I think they were HC+?), and AppCompat wasn't really available to this code. In fact, this whole separate Activity was to work around significant changes in the Preferences APIs Android exposed. Neither of those constraints (AppCompat, old Preferences API) holds true anymore.
So I'll review the code from a technical standpoint, but I'd really rather see all of this get moved into Fennec's settings screens, removing resources from the Services code base and unifying an ugly divide in the settings UI that has outlived its usefulness.
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8908883 [details]
Bug 1398283 - Clean up Sync Preferences screen
https://reviewboard.mozilla.org/r/180508/#review186216
I didn't look too closely at this, but the approach is fine by me, and the details seems fine too.
Can you land the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1375571#c4 as a Pre: to this?
Thanks!
Attachment #8908883 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 14•7 years ago
|
||
I'm not sure that the "Choose what to sync" header actually adds any value, but I don't mind it either. Ryan, yay/nay?
Flags: needinfo?(rfeeley)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8909542 [details]
Bug 1398283 - Pre: Remove unused fxaccount_remote_error_* strings.
https://reviewboard.mozilla.org/r/181010/#review186588
Stamp.
Attachment #8909542 -
Flags: review?(nalexander) → review+
Comment 20•7 years ago
|
||
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3964204faf6
Pre: Remove unused fxaccount_remote_error_* strings. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/0408636c7ea9
Clean up Sync Preferences screen r=nalexander
Comment 21•7 years ago
|
||
Backed out for failing Android lint:
https://hg.mozilla.org/integration/autoland/rev/3f1a3f52cca8a049ab9c15ccb3509e0ea750e664
https://hg.mozilla.org/integration/autoland/rev/ca289939685261cb52551eb87dad5fa7d0b2c067
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0408636c7ea903cc3fa50da56cf5bfcd432421c5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132031356&repo=autoland
Flags: needinfo?(gkruglov)
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d7feb1d2adc
Pre: Remove unused fxaccount_remote_error_* strings. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/9c622588634e
Clean up Sync Preferences screen r=nalexander
Comment 25•7 years ago
|
||
Re: Choose what to sync header… It needs a horizontal line above it (and none between the datatypes)… I'll check the Material design examples and see if that's right.
Flags: needinfo?(rfeeley)
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d7feb1d2adc
https://hg.mozilla.org/mozilla-central/rev/9c622588634e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 27•7 years ago
|
||
Verified as fixed on latest Nightly build with Huawei Honor (Android 5.1.1), Honor 8 (Android 7.0) and Asus ZenPad 8(Android 6.0.1).
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•