Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870773002:
[MD settings] set margins on change download location button (Closed)
|
Created:
3 years, 7 months ago by dschuyler Modified:
3 years, 7 months ago Reviewers:
hcarmona 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] set margins on change download location button
This CL sets the padding/margin on buttons so that there is 12px on either
side of the button label text (per UI design).
BUG=714352
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2870773002
Cr-Commit-Position: refs/heads/master@{#470650}
Committed: https://chromium.googlesource.com/chromium/src/+/bff7fbab75d3308f3901a198853373638b8efcee
Patch Set 1 #
Total comments: 1
Patch Set 2 : merge with master #
Total comments: 3
Messages
Total messages: 23 (15 generated)
Description was changed from ========== [MD settings] set margins on change download location button This CL sets the padding/margin on buttons so that there is 12px on either side of the button label text (per UI design). BUG=714352 ========== to ========== [MD settings] set margins on change download location button This CL sets the padding/margin on buttons so that there is 12px on either side of the button label text (per UI design). BUG=714352 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
dschuyler@chromium.org changed reviewers: + hcarmona@chromium.org
https://codereview.chromium.org/2870773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (left): https://codereview.chromium.org/2870773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:121: -webkit-padding-start: 12px; Moving the padding to the .primary-button and .secondary-button applies the padding to settings-controlled-button as well (which is desired). The other change is moving from using 12/-12px to using a variable.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:120: .settings-box .secondary-button { ------------------------------------------------------------------------ Can we put this here so it's a bit more obvious that the padding and margin are related in both the primary and secondary buttons? --paper-button: { -webkit-padding-end: var(--settings-button-edge-spacing); -webkit-padding-start: var(--settings-button-edge-spacing); }
https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:120: .settings-box .secondary-button { On 2017/05/09 17:32:01, hcarmona wrote: > ------------------------------------------------------------------------ > Can we put this here so it's a bit more obvious that the padding and > margin are related in both the primary and secondary buttons? > > --paper-button: { > -webkit-padding-end: var(--settings-button-edge-spacing); > -webkit-padding-start: var(--settings-button-edge-spacing); > } Like redundantly for emphasis? I'm concerned that may open up including other style info too (I guess I'm making a 'slippery slope' argument, which is a bad argument I acknowledge).
lgtm https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2870773002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:120: .settings-box .secondary-button { On 2017/05/09 18:15:09, dschuyler wrote: > On 2017/05/09 17:32:01, hcarmona wrote: > > ------------------------------------------------------------------------ > > Can we put this here so it's a bit more obvious that the padding and > > margin are related in both the primary and secondary buttons? > > > > --paper-button: { > > -webkit-padding-end: var(--settings-button-edge-spacing); > > -webkit-padding-start: var(--settings-button-edge-spacing); > > } > > Like redundantly for emphasis? I'm concerned that may open up including other > style info too (I guess I'm making a 'slippery slope' argument, which is a bad > argument I acknowledge). No, I looked into mixins. I thought they stacked, but adding a --paper-button here will override the other values. I'd rather not move this up b/c it would cause duplication.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
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": 20001, "attempt_start_ts": 1494439759619450,
"parent_rev": "22b6704f77638e8621311aa2c11e6ea1778885bf", "commit_rev":
"bff7fbab75d3308f3901a198853373638b8efcee"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] set margins on change download location button This CL sets the padding/margin on buttons so that there is 12px on either side of the button label text (per UI design). BUG=714352 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] set margins on change download location button This CL sets the padding/margin on buttons so that there is 12px on either side of the button label text (per UI design). BUG=714352 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2870773002 Cr-Commit-Position: refs/heads/master@{#470650} Committed: https://chromium.googlesource.com/chromium/src/+/bff7fbab75d3308f3901a1988533... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bff7fbab75d3308f3901a1988533... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
