Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801753003:
MD-Settings: Label toggle button in Accessibility section. (Closed)
|
Created:
3 years, 8 months ago by hcarmona Modified:
3 years, 8 months ago 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, groby-ooo-7-16 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD-Settings: Label toggle button in Accessibility section.
BUG=620517
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2801753003
Cr-Commit-Position: refs/heads/master@{#463031}
Committed: https://chromium.googlesource.com/chromium/src/+/e4b092f17442ceb6ff2dbf9899f626f7b908f916
Patch Set 1 #
Total comments: 4
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : remove aria-labelledby #
Total comments: 1
Patch Set 4 : keep div #
Total comments: 2
Messages
Total messages: 36 (22 generated)
Description was changed from ========== MD-Settings: Label toggle button in Accessibility section. BUG=620517 ========== to ========== MD-Settings: Label toggle button in Accessibility section. BUG=620517 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hcarmona@chromium.org changed reviewers: + dschuyler@chromium.org
PTAL, no UI changes
https://codereview.chromium.org/2801753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2801753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:21: <div class="settings-box block"> I don't think .block is helping here. s/block// https://codereview.chromium.org/2801753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <settings-toggle-button id="optionsInMenuToggle" Consider adding .settings-box here <settings-toggle-button id="optionsInMenuToggle" class="settings-box" and removing lines 21 and 27.
The CQ bit was checked by hcarmona@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/2801753003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2801753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:21: <div class="settings-box block"> On 2017/04/06 22:04:53, dschuyler wrote: > I don't think .block is helping here. s/block// Done. https://codereview.chromium.org/2801753003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <settings-toggle-button id="optionsInMenuToggle" On 2017/04/06 22:04:53, dschuyler wrote: > Consider adding .settings-box here > <settings-toggle-button id="optionsInMenuToggle" class="settings-box" > > and removing lines 21 and 27. Sounds good. Done.
https://codereview.chromium.org/2801753003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2801753003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:25: aria-labelledby="optionsInMenuLabel"> please remove this aria-labelledby now that the ID doesn't exist
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 hcarmona@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/2801753003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2801753003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:25: aria-labelledby="optionsInMenuLabel"> On 2017/04/07 00:01:58, Dan Beam wrote: > please remove this aria-labelledby now that the ID doesn't exist Good catch. Removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: class="settings-box" i suppose this could work but we've got a lot of other <div class="settings-box"> <settings-toggle-button ...> </settings-toggle-button> </div> in the rest of the code. class="settings-box" maybe change rules in the future and you might want to stick with how most of the other code works because it might apply styles in a different way than you'd expect without breaking anything
lgtm
On 2017/04/07 18:55:03, Dan Beam wrote: > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): > > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/a11y_page/a11y_page.html:22: > class="settings-box" > i suppose this could work but we've got a lot of other > > <div class="settings-box"> > <settings-toggle-button ...> > </settings-toggle-button> > </div> > > in the rest of the code. class="settings-box" maybe change rules in the future > and you might want to stick with how most of the other code works because it > might apply styles in a different way than you'd expect without breaking > anything Should we change it back, or land as is?
On 2017/04/07 19:22:59, hcarmona wrote: > On 2017/04/07 18:55:03, Dan Beam wrote: > > > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): > > > > > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > > chrome/browser/resources/settings/a11y_page/a11y_page.html:22: > > class="settings-box" > > i suppose this could work but we've got a lot of other > > > > <div class="settings-box"> > > <settings-toggle-button ...> > > </settings-toggle-button> > > </div> > > > > in the rest of the code. class="settings-box" maybe change rules in the > future > > and you might want to stick with how most of the other code works because it > > might apply styles in a different way than you'd expect without breaking > > anything > > Should we change it back, or land as is? In most places we wrap it with a div, but I thought that was a holdover rather than intentional. There is at least one place in category_default_setting.html where the div is not wrapping the settings-toggle-button. Either way is ok with me. I just mean to give some context about my prior input.
On 2017/04/07 19:22:59, hcarmona wrote: > On 2017/04/07 18:55:03, Dan Beam wrote: > > > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): > > > > > https://codereview.chromium.org/2801753003/diff/40001/chrome/browser/resource... > > chrome/browser/resources/settings/a11y_page/a11y_page.html:22: > > class="settings-box" > > i suppose this could work but we've got a lot of other > > > > <div class="settings-box"> > > <settings-toggle-button ...> > > </settings-toggle-button> > > </div> > > > > in the rest of the code. class="settings-box" maybe change rules in the > future > > and you might want to stick with how most of the other code works because it > > might apply styles in a different way than you'd expect without breaking > > anything > > Should we change it back, or land as is? this is what i would've done: https://codereview.chromium.org/2803913006/diff/1/chrome/browser/resources/se... you can do whatever you want; you're on the hook if there's regressions
The CQ bit was checked by hcarmona@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 unchecked by hcarmona@chromium.org
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2801753003/#ps60001 (title: "keep div")
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": 1491598362619950,
"parent_rev": "4a3c245c97370cf05dcbe6e9dae89936042293b1", "commit_rev":
"e4b092f17442ceb6ff2dbf9899f626f7b908f916"}
Message was sent while issue was closed.
Description was changed from ========== MD-Settings: Label toggle button in Accessibility section. BUG=620517 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD-Settings: Label toggle button in Accessibility section. BUG=620517 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2801753003 Cr-Commit-Position: refs/heads/master@{#463031} Committed: https://chromium.googlesource.com/chromium/src/+/e4b092f17442ceb6ff2dbf9899f6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e4b092f17442ceb6ff2dbf9899f6...
Message was sent while issue was closed.
https://codereview.chromium.org/2801753003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (left): https://codereview.chromium.org/2801753003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <div id="optionsInMenuLabel" class="start"> please locally try your changes: http://i.imgur.com/44SADkq.png
Message was sent while issue was closed.
https://codereview.chromium.org/2801753003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.html (left): https://codereview.chromium.org/2801753003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.html:22: <div id="optionsInMenuLabel" class="start"> On 2017/04/11 16:41:31, Dan Beam wrote: > please locally try your changes: http://i.imgur.com/44SADkq.png if it's not clear, the ==0 toggle button is supposed to be aligned at the end, like the |> arrows http://imgur.com/a/1yRPC |
