Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952853005:
Surface sync error icon in account settings UI if sync error (Closed)
DescriptionSurface sync error icon in account settings UI if sync error.
See screenshots in the bug.
BUG=607695
Committed: https://crrev.com/8f0d5ad601adff4d929dd4ec35501cd9c787a0ce
Cr-Commit-Position: refs/heads/master@{#393633}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : add comments #Patch Set 3 : address comments #Patch Set 4 : rebase #Messages
Total messages: 45 (23 generated)
Description was changed from ========== remove redundant head use image surface sync error draft BUG= ========== to ========== Surface sign in sync error icon in account settings UI. BUG=607695 ==========
Description was changed from ========== Surface sign in sync error icon in account settings UI. BUG=607695 ========== to ========== Surface sign in sync error icon in account settings UI. See screenshots in the bug. BUG=607695 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/1
Description was changed from ========== Surface sign in sync error icon in account settings UI. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UI when sync error. See screenshots in the bug. BUG=607695 ==========
Description was changed from ========== Surface sync error icon in account settings UI when sync error. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ==========
Description was changed from ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UIs if sync error. See screenshots in the bug. BUG=607695 ==========
Description was changed from ========== Surface sync error icon in account settings UIs if sync error. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/20001
gogerald@chromium.org changed reviewers: + bauerb@chromium.org, yusufo@chromium.org
Hi, PTAL,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Small UI nit: In the screenshots on the bug, the small icons (Sync/Google G) are centered vertically, but not horizontally (if you compare them to the avatar / add account button). https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_error_widget.xml (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_error_widget.xml:10: android:src="@drawable/sync_error"/> Should we add a textual description for accessibility? https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:257: if (ProfileSyncService.get().getAuthError() == GoogleServiceAuthError.State.NONE) { Would it make sense to move all of this to SyncPreference?
Small UI nit: This is by desgin. https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material... I also don't find a simple way to center icon horizontally in preference except define our own layout or increase assets size. https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_error_widget.xml (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_error_widget.xml:10: android:src="@drawable/sync_error"/> On 2016/05/09 13:58:06, Bernhard Bauer wrote: > Should we add a textual description for accessibility? Done. https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:257: if (ProfileSyncService.get().getAuthError() == GoogleServiceAuthError.State.NONE) { On 2016/05/09 13:58:07, Bernhard Bauer wrote: > Would it make sense to move all of this to SyncPreference? May not, since our design is to indicate sync error to user, so we surfaced sync error as early as in the settings screen, then further signed in account management screen. SyncPreference is for sync configuration.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/09 16:51:59, gogerald1 wrote: > Small UI nit: > This is by desgin. > https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material... OK, thanks. I'll follow up there about this. > I also don't find a simple way to center icon horizontally in preference except > define our own layout or increase assets size. Well, you don't need to increase the size of the rasterized image -- you could define a drawable that contains the image centered, right? https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_error_widget.xml (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_error_widget.xml:10: android:src="@drawable/sync_error"/> On 2016/05/09 16:51:59, gogerald1 wrote: > On 2016/05/09 13:58:06, Bernhard Bauer wrote: > > Should we add a textual description for accessibility? > > Done. It's a good comment, but I was talking about accessibility. In this particular case, we should make sure that we don't exclusively rely on the error icon being visible, because some users might not be able to see that icon (well, or at all), and might for example have to rely on a screen reader. That will require a textual representation though. go/android-a11y has some more information about accessibility. https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:257: if (ProfileSyncService.get().getAuthError() == GoogleServiceAuthError.State.NONE) { On 2016/05/09 16:51:59, gogerald1 wrote: > On 2016/05/09 13:58:07, Bernhard Bauer wrote: > > Would it make sense to move all of this to SyncPreference? > > May not, since our design is to indicate sync error to user, so we surfaced sync > error as early as in the settings screen, then further signed in account > management screen. SyncPreference is for sync configuration. Not really? I'm talking about the SyncPreference class, the instance of which you get in line 256 above. This class configures itself based on the Sync status, so it doesn't make sense to do additional configuration here as well.
Small UI nit: After discussing with UX designer @bettes and PM @ewald, we think we are fine with the current design. https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/sync_error_widget.xml (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/sync_error_widget.xml:10: android:src="@drawable/sync_error"/> On 2016/05/10 09:12:45, Bernhard Bauer wrote: > On 2016/05/09 16:51:59, gogerald1 wrote: > > On 2016/05/09 13:58:06, Bernhard Bauer wrote: > > > Should we add a textual description for accessibility? > > > > Done. > > It's a good comment, but I was talking about accessibility. In this particular > case, we should make sure that we don't exclusively rely on the error icon being > visible, because some users might not be able to see that icon (well, or at > all), and might for example have to rely on a screen reader. That will require a > textual representation though. go/android-a11y has some more information about > accessibility. Done. https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1952853005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:257: if (ProfileSyncService.get().getAuthError() == GoogleServiceAuthError.State.NONE) { On 2016/05/10 09:12:45, Bernhard Bauer wrote: > On 2016/05/09 16:51:59, gogerald1 wrote: > > On 2016/05/09 13:58:07, Bernhard Bauer wrote: > > > Would it make sense to move all of this to SyncPreference? > > > > May not, since our design is to indicate sync error to user, so we surfaced > sync > > error as early as in the settings screen, then further signed in account > > management screen. SyncPreference is for sync configuration. > > Not really? I'm talking about the SyncPreference class, the instance of which > you get in line 256 above. This class configures itself based on the Sync > status, so it doesn't make sense to do additional configuration here as well. Sorry, misunderstood :), done
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping @Yusuf, could PTAL of the changes in sources since Bernhard mentioned that his is the owner of NTP sources only.
lgtm
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1952853005/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952853005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952853005/80001
Message was sent while issue was closed.
Description was changed from ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 ========== to ========== Surface sync error icon in account settings UI if sync error. See screenshots in the bug. BUG=607695 Committed: https://crrev.com/8f0d5ad601adff4d929dd4ec35501cd9c787a0ce Cr-Commit-Position: refs/heads/master@{#393633} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8f0d5ad601adff4d929dd4ec35501cd9c787a0ce Cr-Commit-Position: refs/heads/master@{#393633} |
