MD Settings: Convert spell-check from checkbox to toggle.

MD Settings: Convert spell-check from checkbox to toggle.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2683583008
Cr-Commit-Position: refs/heads/master@{#450099}
Committed: https://chromium.googlesource.com/chromium/src/+/355642b72b189fcc865eeca8d0fcbef8ac91e6e7

[scottchen, 2017-02-08 20:12:35]

Description was changed from

==========
MD Settings: Convert spell-check from checkbox to toggle.

BUG=684152
==========

to

==========
MD Settings: Convert spell-check from checkbox to toggle.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-02-08 20:20:26]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org

[scottchen, 2017-02-08 20:20:27]

dbeam@, I uploaded patch #1 and patch #2 as two potential ways to implement
this. Which one do you think should be the preferred way to do <div> containing
<paper-toggle-buttons>?

Whichever way we agree on, I'll follow as the default way to fix
http://crbug.com/689763 (unless situation warrants otherwise).

[Dan Beam, 2017-02-09 20:11:19]

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

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:70:
.list-item[actionable][disabled] {
i don't think this actually matches anything

did you actually test this code?

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

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:69:
if(!item.language.supportsSpellcheck) {
space after if

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:71: }
no curlies

[scottchen, 2017-02-09 21:30:13]

Thanks for the feedback. I think this CL should still wait to see what we
conclude on in https://codereview.chromium.org/2684853003/

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

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:70:
.list-item[actionable][disabled] {
On 2017/02/09 20:11:18, Dan Beam wrote:
> i don't think this actually matches anything
> 
> did you actually test this code?

Oops, this was left over from patch#1 where I tried to show the approach using
"disabled" attribute.

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

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:69:
if(!item.language.supportsSpellcheck) {
On 2017/02/09 20:11:19, Dan Beam wrote:
> space after if

Done.

https://codereview.chromium.org/2683583008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:71: }
On 2017/02/09 20:11:18, Dan Beam wrote:
> no curlies

Done.

[scottchen, 2017-02-10 23:03:52]

The CQ bit was checked by scottchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:04:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottchen, 2017-02-10 23:49:40]

Update: No longer waiting for the other CL. Based on offline discussion with
dbeam@ and dpapad@, we'll approach these two aspects as two different usages.

[commit-bot: I haz the power, 2017-02-11 00:05:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 00:05:00]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-11 03:10:47]

other than that, lgtm

https://codereview.chromium.org/2683583008/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2683583008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:207:
actionable$="[[item.language.supportsSpellcheck]]">
i think you should move the actionable$= to the exact same node as the on-tap
(to div.start)

[scottchen, 2017-02-13 20:05:52]

thanks, done, landing!

https://codereview.chromium.org/2683583008/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2683583008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:207:
actionable$="[[item.language.supportsSpellcheck]]">
On 2017/02/11 03:10:47, Dan Beam wrote:
> i think you should move the actionable$= to the exact same node as the on-tap
> (to div.start)

Done.

[scottchen, 2017-02-13 20:05:56]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-02-13 20:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2683583008/#ps80001
(title: "Merge branch 'master' into spell-check")

[commit-bot: I haz the power, 2017-02-13 20:06:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 21:35:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487016356775190,
"parent_rev": "9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866", "commit_rev":
"355642b72b189fcc865eeca8d0fcbef8ac91e6e7"}

[commit-bot: I haz the power, 2017-02-13 21:36:01]

Description was changed from

==========
MD Settings: Convert spell-check from checkbox to toggle.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Convert spell-check from checkbox to toggle.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2683583008
Cr-Commit-Position: refs/heads/master@{#450099}
Committed:
https://chromium.googlesource.com/chromium/src/+/355642b72b189fcc865eeca8d0fc...
==========

[commit-bot: I haz the power, 2017-02-13 21:36:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/355642b72b189fcc865eeca8d0fc...