Settings People Revamp: Update main card Sync 'look' to match new spec.

Settings People Revamp: Update main card Sync 'look' to match new spec.

Updates the main card Sync section (visible on Basic page) to match
the updated specs.

BUG=563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0779ffaf01be60ca87d063852134b9673efd2b1e
Cr-Commit-Position: refs/heads/master@{#394606}

[tommycli, 2016-05-17 21:07:13]

Description was changed from

==========
Settings People Revamp: Update main card Sync 'look' to match new spec.

Updates the main card Sync section (visible on Basic page) to match
the updated specs.

BUG=563721
==========

to

==========
Settings People Revamp: Update main card Sync 'look' to match new spec.

Updates the main card Sync section (visible on Basic page) to match
the updated specs.

BUG=563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-05-17 21:11:25]

tommycli@chromium.org changed reviewers:
+ dschuyler@chromium.org

[tommycli, 2016-05-17 21:11:26]

dschuyler: PTAL, thanks!

Before: http://imgur.com/95xzfc4
After: http://imgur.com/6leRjEa

[dschuyler, 2016-05-18 00:08:00]

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:30: /* Per spec,
middle text is indented 20px in this section. */
Thanks for the comment.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:61: .sync-error {
I'm not seeing where this is used.
Can it be removed?

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:207:
!this.syncStatus.syncSystemEnabled) {
Can these reasonably happen?
Or is this action unavailable in these states?

Can we make it so that this action cannot be
initiated (rather than blocked here) in those
situations?

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:237: * @private
@param for syncStatus

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:245: * @private
@param for syncStatus

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:260: * @private
@param

[tommycli, 2016-05-18 18:31:47]

dschuyler: thanks! I addressed your comments below.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:30: /* Per spec,
middle text is indented 20px in this section. */
On 2016/05/18 00:07:59, dschuyler wrote:
> Thanks for the comment.

Acknowledged.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:61: .sync-error {
On 2016/05/18 00:08:00, dschuyler wrote:
> I'm not seeing where this is used.
> Can it be removed?

Done. This sync-error class was returned by the getSyncStatusClass_ method, but
I see now that this is confusing. I modified it to use 'hidden' below. See if
that's clearer.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:207:
!this.syncStatus.syncSystemEnabled) {
On 2016/05/18 00:08:00, dschuyler wrote:
> Can these reasonably happen?
> Or is this action unavailable in these states?
> 
> Can we make it so that this action cannot be
> initiated (rather than blocked here) in those
> situations?

Done. I made the two that can't reasonably happen to be assertions. I made the
one case that can happen to be 'managed'.

Hypothetically, we could remove the event handler if syncStatus.managed, but I
think this is a cleaner solution than to be adding and removing event handlers.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:237: * @private
On 2016/05/18 00:08:00, dschuyler wrote:
> @param for syncStatus

Done.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:245: * @private
On 2016/05/18 00:08:00, dschuyler wrote:
> @param for syncStatus

Done.

https://codereview.chromium.org/1984313003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.js:260: * @private
On 2016/05/18 00:08:00, dschuyler wrote:
> @param

Done.

https://codereview.chromium.org/1984313003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/1984313003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.html:61: .settings-box
.sync-error {
I had to add this to make the class more specific than .settings-box .secondary

https://codereview.chromium.org/1984313003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/1984313003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:245:
!!syncStatus.syncSystemEnabled;
This was necessary to make the closure compiler happy. (force it to be a boolean
instead of {boolean|undefined}

[dschuyler, 2016-05-18 20:33:30]

lgtm

[tommycli, 2016-05-18 21:17:28]

dschuyler: thanks!

[tommycli, 2016-05-18 21:17:31]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-05-18 21:18:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984313003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984313003/20001

[commit-bot: I haz the power, 2016-05-18 23:49:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-18 23:52:38]

Description was changed from

==========
Settings People Revamp: Update main card Sync 'look' to match new spec.

Updates the main card Sync section (visible on Basic page) to match
the updated specs.

BUG=563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Settings People Revamp: Update main card Sync 'look' to match new spec.

Updates the main card Sync section (visible on Basic page) to match
the updated specs.

BUG=563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0779ffaf01be60ca87d063852134b9673efd2b1e
Cr-Commit-Position: refs/heads/master@{#394606}
==========

[commit-bot: I haz the power, 2016-05-18 23:52:39]

Patchset 2 (id:??) landed as
https://crrev.com/0779ffaf01be60ca87d063852134b9673efd2b1e
Cr-Commit-Position: refs/heads/master@{#394606}