Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1732143002:
[MD settings] using paper and google colors in settings (Closed)
|
Created:
4 years, 10 months ago by dschuyler Modified:
4 years, 10 months ago Reviewers:
Dan Beam 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] using paper and google colors in settings
This CL changes some css colors from using #xxxxxx
or rgb(r,g,b) to using either paper or google color
vars. Some of the translations are approximate, where
there is not an exact match for the rgb.
BUG=589630
Committed: https://crrev.com/b45be7d56bd93f9f19dbebcacb6996839023bbcd
Cr-Commit-Position: refs/heads/master@{#377942}
Patch Set 1 #
Total comments: 10
Patch Set 2 : cleanup #Patch Set 3 : review changes #
Total comments: 2
Patch Set 4 : removed text decoration none #Messages
Total messages: 22 (8 generated)
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
lgtm++
The CQ bit was checked by dschuyler@chromium.org
https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:55: paper-button.primary-button { could you just add to this rule, actually paper-button.primary-button, paper-button.tertiary-button { https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:85: color: var(--google-blue-700); ^ and remove this https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:173: } is this a copy... did I do this from a merge conflict maybe?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732143002/1
The CQ bit was unchecked by dschuyler@chromium.org
https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:55: paper-button.primary-button { On 2016/02/25 00:08:17, Dan Beam wrote: > could you just add to this rule, actually > > paper-button.primary-button, > paper-button.tertiary-button { Hmm, the tertiary-button has the color inside a --paper-button:. I think that may behave differently (i.e. are ya sure?). https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:57: * --paper where applicable. */ I'll do a followup to remove this comment as well. https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:85: color: var(--google-blue-700); On 2016/02/25 00:08:17, Dan Beam wrote: > ^ and remove this Acknowledged. https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:173: } On 2016/02/25 00:08:17, Dan Beam wrote: > is this a copy... did I do this from a merge conflict maybe? Done.
https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:55: paper-button.primary-button { On 2016/02/25 00:19:07, dschuyler wrote: > On 2016/02/25 00:08:17, Dan Beam wrote: > > could you just add to this rule, actually > > > > paper-button.primary-button, > > paper-button.tertiary-button { > > Hmm, the tertiary-button has the color inside > a --paper-button:. I think that may behave > differently (i.e. are ya sure?). it is different: it's slower ;) https://codereview.chromium.org/1725183002/diff/20001/chrome/browser/resource... https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:57: * --paper where applicable. */ On 2016/02/25 00:19:07, dschuyler wrote: > I'll do a followup to remove this comment as well. Acknowledged.
https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:55: paper-button.primary-button { On 2016/02/25 00:44:47, Dan Beam wrote: > On 2016/02/25 00:19:07, dschuyler wrote: > > On 2016/02/25 00:08:17, Dan Beam wrote: > > > could you just add to this rule, actually > > > > > > paper-button.primary-button, > > > paper-button.tertiary-button { > > > > Hmm, the tertiary-button has the color inside > > a --paper-button:. I think that may behave > > differently (i.e. are ya sure?). > > it is different: it's slower ;) > > https://codereview.chromium.org/1725183002/diff/20001/chrome/browser/resource... Done.
still lgtm https://codereview.chromium.org/1732143002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:140: text-decoration: none; fwiw: I'm not sure text-decoration: none; is needed
Removed that text-decoration: none. https://codereview.chromium.org/1732143002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/1732143002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_shared_css.html:140: text-decoration: none; On 2016/02/25 22:28:09, Dan Beam wrote: > fwiw: I'm not sure text-decoration: none; is needed Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1732143002/#ps60001 (title: "removed text decoration none")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732143002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] using paper and google colors in settings This CL changes some css colors from using #xxxxxx or rgb(r,g,b) to using either paper or google color vars. Some of the translations are approximate, where there is not an exact match for the rgb. BUG=589630 ========== to ========== [MD settings] using paper and google colors in settings This CL changes some css colors from using #xxxxxx or rgb(r,g,b) to using either paper or google color vars. Some of the translations are approximate, where there is not an exact match for the rgb. BUG=589630 Committed: https://crrev.com/b45be7d56bd93f9f19dbebcacb6996839023bbcd Cr-Commit-Position: refs/heads/master@{#377942} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b45be7d56bd93f9f19dbebcacb6996839023bbcd Cr-Commit-Position: refs/heads/master@{#377942} |
