Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052573003:
MD Settings: Update spell check section to spec (Closed)
|
Created:
4 years, 6 months ago by michaelpg Modified:
4 years, 6 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@LanguagePolish Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Update spell check section to spec
Replaces paper-checkboxes with paper-toggle-buttons.
Shows all languages and disables the ones that don't support spell check.
Adds some common label styling to settings_shared_css.html.
Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1
BUG=599993
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/40aea5f055f5ad469dc3692db2cc3c42eb520f09
Cr-Commit-Position: refs/heads/master@{#400888}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : comments #
Total comments: 7
Patch Set 4 : styling #
Messages
Total messages: 18 (8 generated)
Description was changed from ========== MD Settings: Update spell check section to spec BUG=599993 R=dschuyler@chromium.org ========== to ========== MD Settings: Update spell check section to spec BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Update spell check section to spec BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:139: </label> .start in the .settings-box is referring to the 'stuff on the left' in LTR (or on the right in RTL). I'm hoping to keep that in the lists. To be more consistent with the settings-box .start Please make the .start be expanding flex part and move the toggle button outside of start. e.g. <label class="start"> <span ... </span> </label> <paper-toggle-button ... </paper-toggle-button> https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:192: display: flex; Can we move the flex: 1 here and remove the .list-item > label.start > span {
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
PTAL. BTW, screenshot at https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:139: </label> On 2016/06/10 21:49:46, dschuyler wrote: > .start in the .settings-box is referring to the > 'stuff on the left' in LTR (or on the right in RTL). > I'm hoping to keep that in the lists. > > To be more consistent with the settings-box .start > Please make the .start be expanding flex part and move > the toggle button outside of start. e.g. > <label class="start"> > <span ... > </span> > </label> > <paper-toggle-button ... > </paper-toggle-button> alright, i moved the paper-toggle-button outside the <label>. that required dynamically setting the label ID. https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2052573003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:192: display: flex; On 2016/06/10 21:49:46, dschuyler wrote: > Can we move the flex: 1 here and > remove the > .list-item > label.start > span { Done.
Description was changed from ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:132: <span disabled$="[[!item.language.supportsSpellcheck]]"> Can we disable the label? (Is the nesting into a <span> necessary)? https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:207: .list-item > label.start { Do we need 'label', can this be .list-item > .start https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:214: opacity: .5; /* TODO(michaelpg): check this value with bettes. */ It's still a good idea to check with bettes@ when he's back, but let's go with the Polymer styling for this until Alan says otherwise. That would be .65 in place of the .5. e.g. https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:132: <span disabled$="[[!item.language.supportsSpellcheck]]"> On 2016/06/20 22:46:31, dschuyler wrote: > Can we disable the label? (Is the > nesting into a <span> necessary)? I specifically don't want to, because it's common practice to nest input controls inside labels. In which case, we'd still have to set the disabled attributes on Polymer elements, and the opacity styles from the label and the Polymer element would stack. May not be an issue in practice. LMKWYT. https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:207: .list-item > label.start { On 2016/06/20 22:46:31, dschuyler wrote: > Do we need 'label', can this be > .list-item > .start hmm, I think this is obsolete now https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:214: opacity: .5; /* TODO(michaelpg): check this value with bettes. */ On 2016/06/20 22:46:31, dschuyler wrote: > It's still a good idea to check with bettes@ > when he's back, but let's go with the Polymer > styling for this until Alan says otherwise. > That would be .65 in place of the .5. > > e.g. > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... Done.
lgtm https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:132: <span disabled$="[[!item.language.supportsSpellcheck]]"> On 2016/06/20 23:09:34, michaelpg wrote: > On 2016/06/20 22:46:31, dschuyler wrote: > > Can we disable the label? (Is the > > nesting into a <span> necessary)? > > I specifically don't want to, because it's common practice to nest input > controls inside labels. In which case, we'd still have to set the disabled > attributes on Polymer elements, and the opacity styles from the label and the > Polymer element would stack. > > May not be an issue in practice. LMKWYT. Just an opinion, but I'd rather that it wasn't common practice to put controls in something called a label :) Thank you for explaining the reason for it.
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2052573003/100001
On 2016/06/20 23:46:25, dschuyler wrote: > lgtm > > https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/settings/languages_page/languages_page.html > (right): > > https://codereview.chromium.org/2052573003/diff/80001/chrome/browser/resource... > chrome/browser/resources/settings/languages_page/languages_page.html:132: <span > disabled$="[[!item.language.supportsSpellcheck]]"> > On 2016/06/20 23:09:34, michaelpg wrote: > > On 2016/06/20 22:46:31, dschuyler wrote: > > > Can we disable the label? (Is the > > > nesting into a <span> necessary)? > > > > I specifically don't want to, because it's common practice to nest input > > controls inside labels. In which case, we'd still have to set the disabled > > attributes on Polymer elements, and the opacity styles from the label and the > > Polymer element would stack. > > > > May not be an issue in practice. LMKWYT. > > Just an opinion, but I'd rather that it > wasn't common practice to put controls > in something called a label :) Thank > you for explaining the reason for it. Thanks. I don't care one way or the other wrt labeling, but the CSS here is "safer". Feel free to adjust the markup next time you're poking around in here!
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Update spell check section to spec Replaces paper-checkboxes with paper-toggle-buttons. Shows all languages and disables the ones that don't support spell check. Adds some common label styling to settings_shared_css.html. Screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=239710&inline=1 BUG=599993 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/40aea5f055f5ad469dc3692db2cc3c42eb520f09 Cr-Commit-Position: refs/heads/master@{#400888} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/40aea5f055f5ad469dc3692db2cc3c42eb520f09 Cr-Commit-Position: refs/heads/master@{#400888} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
