Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902363002:
[MD settings] adjust button layout (Closed)
|
Created:
3 years, 7 months ago by dschuyler Modified:
3 years, 6 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] adjust button layout
This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator.
BUG=725172, 726262, 724944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2902363002
Cr-Commit-Position: refs/heads/master@{#476067}
Committed: https://chromium.googlesource.com/chromium/src/+/e959cddaeca101036a8edf445a5d57c629632afc
Patch Set 1 #Patch Set 2 : icon buttons #
Total comments: 2
Patch Set 3 : restore overflow-y auto #
Total comments: 9
Patch Set 4 : review changes #Patch Set 5 : moved styling #
Total comments: 6
Patch Set 6 : nits #Patch Set 7 : merge with master #
Messages
Total messages: 59 (38 generated)
Description was changed from ========== [MD settings] adjust button layout BUG= ========== to ========== [MD settings] adjust button layout BUG= 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [MD settings] adjust button layout BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] adjust button layout This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator. BUG=725172 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
Here's another pass and getting the button spacing laid out. As mentioned in the CL description, there are several cases to consider. WDYT of the approach in patch 1 of this CL?
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...
https://codereview.chromium.org/2902363002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/controls/controlled_button.html (right): https://codereview.chromium.org/2902363002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/controls/controlled_button.html:17: -webkit-margin-start: calc(var(--settings-button-edge-spacing) * -1); This should be modified to take the separator into account. https://codereview.chromium.org/2902363002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_style_css.html (left): https://codereview.chromium.org/2902363002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:96: overflow-y: auto; I'm concerned about this because I don't have a full picture of the impact of this change. The reason it was changed was to get full paper ripples (avoid cutting off the right edge of ripples) in the onStartup url list. I haven't seen any issues with removing it yet.
On 2017/05/25 21:30:24, dschuyler wrote: > https://codereview.chromium.org/2902363002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/controls/controlled_button.html (right): > > https://codereview.chromium.org/2902363002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/controls/controlled_button.html:17: > -webkit-margin-start: calc(var(--settings-button-edge-spacing) * -1); > This should be modified to take the separator into account. > > https://codereview.chromium.org/2902363002/diff/20001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/shared_style_css.html (left): > > https://codereview.chromium.org/2902363002/diff/20001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/shared_style_css.html:96: overflow-y: auto; > I'm concerned about this because I don't have a full picture of the impact of > this change. The reason it was changed was to get full paper ripples (avoid > cutting off the right edge of ripples) in the onStartup url list. I haven't seen > any issues with removing it yet. Patch #2 has been posted for the sake of discussion. (Not as a be-all-solution).
So, ultimately I'm probably not the best person to review this; my CSS fu is not very strong. (Also FWIW I am OOO tomorrow). Two comments though: 1) I'm a little concerned about putting specific layout/spacing rules in the cr shared styling. 2) My ideal solution would be to modify the styling of all paper-buttons (and maybe paper-icon-buttons) so that the edges of the element align with the visible part of the element (i.e. the text), by applying a negative left/right margin to all paper buttons. The problem however is that then we don't have a way to separate buttons. Maybe we can make the right/end margin aligned with the text edge, and have spacing on the left/start side? I'm not sure what other problems that might cause though. 3) The subtle CSS rules here concern me some, but maybe they shouldn't? See my initial caveat about weak CSS fu.
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...
https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html (right): https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:27: } We might want to do this separately, it's not completely obviously related. Also, what happens if there are only a couple of startup urls?
dschuyler@chromium.org changed reviewers: + hcarmona@chromium.org - stevenjb@chromium.org
stevenjb@ will be OOO so handing off reviewer to hcarmona@
Description was changed from ========== [MD settings] adjust button layout This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator. BUG=725172 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] adjust button layout This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator. BUG=725172, 726262, 724944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
I'm also concerned by Steven's concern about putting specific layout/spacing rules in the cr-shared styling. Do the buttons outside of settings also need this? https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:55: .settings-box paper-button:last-of-type { This will also affect a button that is by itself. Just making sure it's intentional. https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_style_css.html (right): https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:52: -webkit-margin-end: calc(var(--cr-icon-ripple-padding) * -1); This is used in 4 places, can we make a variable for var(--cr-icon-ripple-padding) * -1 ? possibly --cr-icon-ripple-margin https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:88: paper-icon-button { Can this https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:93: paper-icon-button { and this can be combined b/c they use the same selector?
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...
On 2017/05/25 23:42:59, hcarmona wrote: > I'm also concerned by Steven's concern about putting specific layout/spacing > rules in the cr-shared styling. I've added https://bugs.chromium.org/p/chromium/issues/detail?id=726546 to follow-up on that.
https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2902363002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:55: .settings-box paper-button:last-of-type { On 2017/05/25 23:42:58, hcarmona wrote: > This will also affect a button that is by itself. > Just making sure it's intentional. If the button is entirely by itself or if it has stuff before it, that should all be fine iiuc. If there is only one button with (or if the last button has) stuff after it, that may be an issue (and I intent to dig into this further after the m60 branch). https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_style_css.html (right): https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:52: -webkit-margin-end: calc(var(--cr-icon-ripple-padding) * -1); On 2017/05/25 23:42:58, hcarmona wrote: > This is used in 4 places, can we make a variable for > var(--cr-icon-ripple-padding) * -1 ? > possibly --cr-icon-ripple-margin Done. https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:88: paper-icon-button { On 2017/05/25 23:42:58, hcarmona wrote: > Can this Done. https://codereview.chromium.org/2902363002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_style_css.html:93: paper-icon-button { On 2017/05/25 23:42:58, hcarmona wrote: > and this can be combined b/c they use the same selector? Done.
lgtm
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I reconsidered the changes to cr shared css and reduced what is changing in that file. I'll want to revisit that later, but since I want to merge this CL into m60, I'm being more cautious.
Still LGTM, just some nits. https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:37: .separator + button[is='paper-icon-button-light'] { Nit: combine this https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:41: .separator + paper-icon-button { and this b/c they apply the same styling https://codereview.chromium.org/2902363002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2902363002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_vars_css.html:27: --cr-paper-icon-button-margin: { Nit: Use --cr-icon-ripple-margin here or use --cr-icon-ripple-margin here in the two places this is used? (This can be a separate CL to keep this small)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:37: .separator + button[is='paper-icon-button-light'] { On 2017/05/26 19:00:50, hcarmona wrote: > Nit: combine this Done. https://codereview.chromium.org/2902363002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:41: .separator + paper-icon-button { On 2017/05/26 19:00:50, hcarmona wrote: > and this b/c they apply the same styling Done. https://codereview.chromium.org/2902363002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2902363002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_vars_css.html:27: --cr-paper-icon-button-margin: { On 2017/05/26 19:00:50, hcarmona wrote: > Nit: Use --cr-icon-ripple-margin here or use --cr-icon-ripple-margin here in the > two places this is used? > > (This can be a separate CL to keep this small) Done.
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 dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2902363002/#ps120001 (title: "merge with master")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dschuyler@chromium.org changed reviewers: + tommycli@chromium.org
tommycli@ for OWNER on cr_elements files.
On 2017/05/26 23:08:02, dschuyler wrote: > tommycli@ for OWNER on cr_elements files. rs lgtm assuming hector has already approved the cr_elements changes
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
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": 120001, "attempt_start_ts": 1496269456506480,
"parent_rev": "53b8275babae3275605339cc09038d1dfbcf3ec0", "commit_rev":
"e959cddaeca101036a8edf445a5d57c629632afc"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] adjust button layout This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator. BUG=725172, 726262, 724944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] adjust button layout This CL gives proper spacing of paper-buttons in settings boxes. There are several cases to consider: a button at the start of a row; a button at the end of the row; multiple buttons in a row; buttons before or after a separator. BUG=725172, 726262, 724944 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2902363002 Cr-Commit-Position: refs/heads/master@{#476067} Committed: https://chromium.googlesource.com/chromium/src/+/e959cddaeca101036a8edf445a5d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e959cddaeca101036a8edf445a5d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
