|
|
Created:
3 years, 8 months ago by dschuyler Modified:
3 years, 8 months ago Reviewers:
stevenjb 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] clickable toggle labels on category default toggles
This CL makes top level category default rows react to mouse clicks.
These are the one or two rows at the top of a content settings category
(such as "cookies"). The rows are toggles. Before this CL the labels on
the toggles wouldn't act on mouse clicks when they should.
Also, there are several name changes and tiddying up that could be in a
separate CL if that is preferred.
BUG=704003
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2788853003
Cr-Commit-Position: refs/heads/master@{#461533}
Committed: https://chromium.googlesource.com/chromium/src/+/ae7fc3094d50f04c513f4b62c5f45475cbba17f5
Patch Set 1 #
Total comments: 8
Patch Set 2 : improved comments #
Total comments: 2
Patch Set 3 : restore separator line #
Messages
Total messages: 26 (16 generated)
Description was changed from ========== [MD settings] clickable toggle labels on category default toggles This CL makes top level category default rows react to mouse clicks. These are the one or two rows at the top of a content settings category (such as "cookies"). The rows are toggles. Before this CL the labels on the toggles wouldn't act on mouse clicks when they should. Also, there are several name changes and tiddying up that could be in a separate CL if that is preferred. BUG=704003 ========== to ========== [MD settings] clickable toggle labels on category default toggles This CL makes top level category default rows react to mouse clicks. These are the one or two rows at the top of a content settings category (such as "cookies"). The rows are toggles. Before this CL the labels on the toggles wouldn't act on mouse clicks when they should. Also, there are several name changes and tiddying up that could be in a separate CL if that is preferred. BUG=704003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
On 2017/03/31 23:15:45, dschuyler wrote: It's hard to describe where in the UI this change is. Please see screen shots in bug (a picture is better for this).
https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:13: class="settings-box first" Interesting. I guess since settings-toggle-button is kind of an uber control, designed to span an entire row, putting the layout here instead of in a surrounding div makes sense (and obviously saves a div, for what that's worth). It hurts my brain a little, but seems fine? https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:16: disabled$="[[isToggleDisabled_(category)]]"> I forget, do we need to explicitly set a11y properties for settings-toggle-button? https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.js:23: toggleOnLabel: String, Maybe move these above optionDescription and mention that they are used to set optionLabel_ below? That way we expain option label, option desc, sub-option label, sub-option desc. https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.js:31: * The second line (secondary) text to be shown next to the toggle. All three of these have the same comment.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:13: class="settings-box first" On 2017/03/31 23:28:14, stevenjb wrote: > Interesting. I guess since settings-toggle-button is kind of an uber control, > designed to span an entire row, putting the layout here instead of in a > surrounding div makes sense (and obviously saves a div, for what that's worth). > It hurts my brain a little, but seems fine? It should be good. You're right, the whole row is a toggle. https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:16: disabled$="[[isToggleDisabled_(category)]]"> On 2017/03/31 23:28:14, stevenjb wrote: > I forget, do we need to explicitly set a11y properties for > settings-toggle-button? Iiuc, it's implicit, handled by settings_toggle_button.html internally. https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.js:23: toggleOnLabel: String, On 2017/03/31 23:28:15, stevenjb wrote: > Maybe move these above optionDescription and mention that they are used to set > optionLabel_ below? That way we expain option label, option desc, sub-option > label, sub-option desc. I didn't do exactly that. I've sorted the public above the @privates. I've also tried to re-write the comments to be clearer. PTAL, WDYT? https://codereview.chromium.org/2788853003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.js:31: * The second line (secondary) text to be shown next to the toggle. On 2017/03/31 23:28:15, stevenjb wrote: > All three of these have the same comment. Whoops, thanks!
lgtm https://codereview.chromium.org/2788853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2788853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:28: toggleOnLabel: String, This is fine, but I still personally think label, desc, sublabel, subdesc would be easier to follow. *shrug*
https://codereview.chromium.org/2788853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2788853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:28: toggleOnLabel: String, On 2017/04/01 00:15:37, stevenjb wrote: > This is fine, but I still personally think label, desc, sublabel, subdesc would > be easier to follow. *shrug* Seems reasonable. I'd like to rework this code to make the second (desc) line change with the toggle on/off and keep the first (label) line consistent. I'll keep this in mind.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I realized that I should not have put the .continuation class on the second toggle. So I've removed it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491243353636110, "parent_rev": "b089e9dc70ad94b569d54441e01668090a128d6d", "commit_rev": "ae7fc3094d50f04c513f4b62c5f45475cbba17f5"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] clickable toggle labels on category default toggles This CL makes top level category default rows react to mouse clicks. These are the one or two rows at the top of a content settings category (such as "cookies"). The rows are toggles. Before this CL the labels on the toggles wouldn't act on mouse clicks when they should. Also, there are several name changes and tiddying up that could be in a separate CL if that is preferred. BUG=704003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] clickable toggle labels on category default toggles This CL makes top level category default rows react to mouse clicks. These are the one or two rows at the top of a content settings category (such as "cookies"). The rows are toggles. Before this CL the labels on the toggles wouldn't act on mouse clicks when they should. Also, there are several name changes and tiddying up that could be in a separate CL if that is preferred. BUG=704003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2788853003 Cr-Commit-Position: refs/heads/master@{#461533} Committed: https://chromium.googlesource.com/chromium/src/+/ae7fc3094d50f04c513f4b62c5f4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ae7fc3094d50f04c513f4b62c5f4... |