New issue
Advanced search Search tips

Issue 709113 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Add checkbox for "Control how this works in Settings" when MD settings is enabled

Project Member Reported by ew...@chromium.org, Apr 6 2017

Issue description

Per offline discussion, when MD settings is enabled, we should use a checkbox for "Control how this works in Settings" instead of link-ifying [Settings]. This will make it more clear for users that want to go configure sync settings in the advanced sync settings subpage that after they leave that subpage, sync will start.
 

Comment 1 by ew...@chromium.org, Apr 6 2017

Labels: -Restrict-View-Google

Comment 2 by dbeam@chromium.org, Apr 6 2017

Cc: rogerta@chromium.org
screenshots of in-progress implementation
2017-04-06-114021_458x495_scrot.png
42.5 KB View Download
2017-04-06-111108_456x498_scrot.png
42.8 KB View Download
2017-04-06-111116_457x498_scrot.png
43.3 KB View Download
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cafbdba2f02dddaa020d228b591051692bcae535

commit cafbdba2f02dddaa020d228b591051692bcae535
Author: dbeam <dbeam@chromium.org>
Date: Thu Apr 06 23:22:26 2017

Sync confirmation: change "Settings" link to checkbox

This is so all users must confirm sign in via "OK, GOT IT"
instead of clicking "Settings" link (which closes the dialog
without either of the "UNDO" or "OK, GOT IT" buttons being
clicked).

R=rogerta@chromium.org
BUG= 709113 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2800683008
Cr-Commit-Position: refs/heads/master@{#462675}

[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/app/generated_resources.grd
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/resources/signin/sync_confirmation/sync_confirmation.js
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/sync/one_click_signin_sync_starter.h
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/webui/signin/sync_confirmation_handler.cc
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/webui/signin/sync_confirmation_handler.h
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
[modify] https://crrev.com/cafbdba2f02dddaa020d228b591051692bcae535/chrome/browser/ui/webui/signin/sync_confirmation_ui.cc

Comment 4 Deleted

Labels: -crbug.com709113 Hotlist-MD-Settings-UIReview
Let's make sure that the checkbox text is aligned with the other text and of the same color, weight, and size of the secondary body text of this dialog. Thanks!



preview.png
270 KB View Download
spec.png
179 KB View Download

Comment 7 by ew...@chromium.org, Apr 7 2017

Also, just wanted to double check: we decided that this checkbox should only be shown if MD settings is  enabled, right?

Comment 8 by dbeam@chromium.org, Apr 7 2017

that's what we decided but i accidentally just made it so for all cases.

if we launch MD settings in M59, i'll leave everything as is.

if we don't, I'll revert this specific CL on the M58 branch.

Comment 9 by dbeam@chromium.org, Apr 7 2017

correction: I'll revert the CL on the **59** branch
#8-9 SGTM

One more nit: I noticed on Mac that the checkbox seems to temporarily get focus as the animation is finishing. Attaching a gif for reference.
checkbox-focus.gif
214 KB View Download
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbe458bd546647113889eb95afdba9cce3d8b236

commit fbe458bd546647113889eb95afdba9cce3d8b236
Author: dbeam <dbeam@chromium.org>
Date: Sat Apr 08 00:19:58 2017

Sync confirmation: tweak "Control how this works in settings" checkbox spacing

R=tommycli@chromium.org
BUG= 709113 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2803373002
Cr-Commit-Position: refs/heads/master@{#463068}

[modify] https://crrev.com/fbe458bd546647113889eb95afdba9cce3d8b236/chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html

Labels: -Pri-2 Pri-3
Status: Fixed (was: Assigned)
please file a separate bug about changing the string if we find something better
Labels: -Pri-3 Pri-2
Do we think that the extraneous focus issue on Mac mentioned in #10 is minor enough to leave be for now? Would you like me to file a separate bug for it instead?

Sign in to add a comment