Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821383002:
MD Settings: make [un]controlled radio buttons actionable (cursor: pointer) (Closed)
|
Created:
3 years, 8 months ago by Dan Beam Modified:
3 years, 8 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: make [un]controlled radio buttons actionable (cursor: pointer)
R=dpapad@chromium.org
BUG=712601
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2821383002
Cr-Commit-Position: refs/heads/master@{#465334}
Committed: https://chromium.googlesource.com/chromium/src/+/e05fcba620c2612ed8a7976134ae02fb4781d4e4
Patch Set 1 #
Total comments: 4
Messages
Total messages: 16 (8 generated)
Description was changed from ========== MD Settings: make [un]controlled radio buttons actionable (cursor: pointer) R=dpapad@chromium.org BUG=712601 ========== to ========== MD Settings: make [un]controlled radio buttons actionable (cursor: pointer) R=dpapad@chromium.org BUG=712601 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dbeam@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...
https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:86: :host(:not([controlled_])) { Can radio buttons be disabled otherwise? For example with a |disabled| attribute? Would it be more robust as follows? :host { .... @apply(--settings-actionable); } :host([controlled_]), :host([disabled]) { cursor: initial; pointer-events: none; }
https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:86: :host(:not([controlled_])) { On 2017/04/18 18:58:32, dpapad wrote: > Can radio buttons be disabled otherwise? For example with a |disabled| > attribute? > > Would it be more robust as follows? > > :host { > .... > @apply(--settings-actionable); > } > > :host([controlled_]), > :host([disabled]) { > cursor: initial; > pointer-events: none; > } > > > no
lgtm
https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:86: :host(:not([controlled_])) { On 2017/04/18 19:00:37, Dan Beam wrote: > On 2017/04/18 18:58:32, dpapad wrote: > > Can radio buttons be disabled otherwise? For example with a |disabled| > > attribute? > > > > Would it be more robust as follows? > > > > :host { > > .... > > @apply(--settings-actionable); > > } > > > > :host([controlled_]), > > :host([disabled]) { > > cursor: initial; > > pointer-events: none; > > } > > > > > > > > no that was the short answer. the longer answer is that assuming what will be in a CSS mixin to override it is not the best strategy, imo. it also makes less sense to me to apply a style then override it later... instead of only applying it when needed.
https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2821383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:86: :host(:not([controlled_])) { On 2017/04/18 19:10:49, Dan Beam wrote: > On 2017/04/18 19:00:37, Dan Beam wrote: > > On 2017/04/18 18:58:32, dpapad wrote: > > > Can radio buttons be disabled otherwise? For example with a |disabled| > > > attribute? > > > > > > Would it be more robust as follows? > > > > > > :host { > > > .... > > > @apply(--settings-actionable); > > > } > > > > > > :host([controlled_]), > > > :host([disabled]) { > > > cursor: initial; > > > pointer-events: none; > > > } > > > > > > > > > > > > > no > > that was the short answer. the longer answer is that assuming what will be in a > CSS mixin to override it is not the best strategy, imo. it also makes less > sense to me to apply a style then override it later... instead of only applying > it when needed. also, i'm not really sure how [disabled] works in the context. most of this file uses a private |controlled_| and determines it's own fate, whereas disabled (if it's public, especially) can be set by the outside. we generally want to keep things private / up to the minimal scope possible until needed otherwise (i.e. something outside of this class needs to disable this for reasons a <controlled-radio-button> shouldn't really know about)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dbeam@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": 1, "attempt_start_ts": 1492544425251440, "parent_rev":
"1e3f3210c07f758054460e5ca0c1bd46f9f13a3e", "commit_rev":
"e05fcba620c2612ed8a7976134ae02fb4781d4e4"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: make [un]controlled radio buttons actionable (cursor: pointer) R=dpapad@chromium.org BUG=712601 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: make [un]controlled radio buttons actionable (cursor: pointer) R=dpapad@chromium.org BUG=712601 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2821383002 Cr-Commit-Position: refs/heads/master@{#465334} Committed: https://chromium.googlesource.com/chromium/src/+/e05fcba620c2612ed8a7976134ae... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e05fcba620c2612ed8a7976134ae... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
