Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889493002:
[MD settings] removing secondary-button class (Closed)
|
Created:
3 years, 7 months ago by dschuyler Modified:
3 years, 7 months ago Reviewers:
stevenjb CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] removing secondary-button class
This CL removes the global .secondary-button class. At one point we had
several types of buttons, but we eventually narrowed down to two buttons:
.primary-button and .secondary-button; though some buttons were not
specified as either. That meant we effectively had three button classes.
Rather than add .secondary-button to each paper-button that isn't
explicitly secondary already, I've removed that class. Now there are
.primary-buttons and normal buttons; i.e. any non-primary-button is a
secondary button (aka normal) button.
Note: The references to .secondary-button are still in place (based on
Steven's review comment below) and there are a couple places where a
.secondary-button is used locally within a dom-module; those have been
left unchanged.
BUG=721268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2889493002
Cr-Commit-Position: refs/heads/master@{#472301}
Committed: https://chromium.googlesource.com/chromium/src/+/2c8d005df63e8ef30df1ed4f0afe469cca335b2c
Patch Set 1 #
Total comments: 13
Patch Set 2 : review changes #Messages
Total messages: 25 (18 generated)
Description was changed from ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. BUG=721268 ========== to ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Description was changed from ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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
The main change is in chrome/browser/resources/settings/settings_shared_css.html The other files changed to remove the use of .secondary-button.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_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_...)
Suggestion (feel free to ignore): Maybe just clean up the paper-button and .primary-button CSS and remove the .secondary-button shared CSS, but leave the secondary-button class labels for now. Then we can play with the UI for a week or so and make sure we don't see any unexpected artifacts (e.g. buttons that currently are not .secondary-button but look incorrect as secondary buttons). Then we can remove the (now ignored) secondary-button class as a separate CL. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:42: } Can we merge this with the other paper-button CSS? https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:46: } And maybe move this down also (or move the rest up here)? https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:60: } Ditto https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:70: } Ditto https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:102: -webkit-padding-start: var(--settings-button-edge-spacing); Same as for paper-button (and therefor not needed)? https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:103: font-weight: 500; ditto?
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/16 16:27:30, stevenjb wrote: > Suggestion (feel free to ignore): Maybe just clean up the paper-button and > .primary-button CSS and remove the .secondary-button shared CSS, but leave the > secondary-button class labels for now. Then we can play with the UI for a week > or so and make sure we don't see any unexpected artifacts (e.g. buttons that > currently are not .secondary-button but look incorrect as secondary buttons). > Then we can remove the (now ignored) secondary-button class as a separate CL. Great idea!
https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:42: } On 2017/05/16 16:27:29, stevenjb wrote: > Can we merge this with the other paper-button CSS? Done. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:46: } On 2017/05/16 16:27:29, stevenjb wrote: > And maybe move this down also (or move the rest up here)? Done. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:60: } On 2017/05/16 16:27:29, stevenjb wrote: > Ditto Done. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:70: } On 2017/05/16 16:27:29, stevenjb wrote: > Ditto Done. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:102: -webkit-padding-start: var(--settings-button-edge-spacing); On 2017/05/16 16:27:29, stevenjb wrote: > Same as for paper-button (and therefor not needed)? This is in a mixin variable. So it must be a complete set of styles. i.e. this is not like += it's more like = it that it overwrites the prior value. https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:103: font-weight: 500; On 2017/05/16 16:27:29, stevenjb wrote: > ditto? intractable.
Much easier to read now, thanks! lgtm https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2889493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:102: -webkit-padding-start: var(--settings-button-edge-spacing); On 2017/05/16 19:07:56, dschuyler wrote: > On 2017/05/16 16:27:29, stevenjb wrote: > > Same as for paper-button (and therefor not needed)? > > This is in a mixin variable. So it must be a complete set of styles. i.e. this > is not like += it's more like = it that it overwrites the prior value. Ah, right. Might be worth defining a var for these, but maybe not. Up to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dschuyler@chromium.org
The CQ bit was unchecked by dschuyler@chromium.org
Description was changed from ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: The references to .secondary-button are still in place (based on Steven's review comment below) and there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: The references to .secondary-button are still in place (based on Steven's review comment below) and there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: The references to .secondary-button are still in place (based on Steven's review comment below) and there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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": 1494982018821430,
"parent_rev": "e876159e6a71b276bd596c115776623faeba9b47", "commit_rev":
"2c8d005df63e8ef30df1ed4f0afe469cca335b2c"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: The references to .secondary-button are still in place (based on Steven's review comment below) and there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] removing secondary-button class This CL removes the global .secondary-button class. At one point we had several types of buttons, but we eventually narrowed down to two buttons: .primary-button and .secondary-button; though some buttons were not specified as either. That meant we effectively had three button classes. Rather than add .secondary-button to each paper-button that isn't explicitly secondary already, I've removed that class. Now there are .primary-buttons and normal buttons; i.e. any non-primary-button is a secondary button (aka normal) button. Note: The references to .secondary-button are still in place (based on Steven's review comment below) and there are a couple places where a .secondary-button is used locally within a dom-module; those have been left unchanged. BUG=721268 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2889493002 Cr-Commit-Position: refs/heads/master@{#472301} Committed: https://chromium.googlesource.com/chromium/src/+/2c8d005df63e8ef30df1ed4f0afe... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2c8d005df63e8ef30df1ed4f0afe... |
