Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959543002:
Settings: Users: Fix margins for restrict signins list (Closed)
|
Created:
3 years, 6 months ago by stevenjb Modified:
3 years, 6 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/branch-heads/3112 Project:
chromium Visibility:
Public. |
DescriptionSettings: Users: Fix margins for restrict signins list
BUG=730957
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/2959543002
Cr-Commit-Position: refs/branch-heads/3112@{#461}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
Committed: https://chromium.googlesource.com/chromium/src/+/bd0beb1515603576b1e7d12e4351cc5bc4cfd1c8
Patch Set 1 #
Total comments: 3
Messages
Total messages: 17 (8 generated)
Description was changed from ========== Settings: Users: Fix margins for restrict signins list BUG=730957 ========== to ========== Settings: Users: Fix margins for restrict signins list BUG=730957 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org, dschuyler@chromium.org
Note: This is for refs/branch-heads/3112 only
https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/user_list.html (right): https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/user_list.html:40: -webkit-margin-start: var(--cr-icon-button-margin-start); --cr-icon-button-margin-start is defined in shared_vars_css.html. Even if we do depend on it transitively from settings_shared_css.html, we should import it directly, no (include-what-you-use principle). https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/shared_va...
https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/user_list.html (right): https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/user_list.html:40: -webkit-margin-start: var(--cr-icon-button-margin-start); On 2017/06/23 23:31:28, dpapad wrote: > --cr-icon-button-margin-start is defined in shared_vars_css.html. Even if we do > depend on it transitively from settings_shared_css.html, we should import it > directly, no (include-what-you-use principle). > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/shared_va... We do not currently have any examples of a direct include of shared_vars_css.html in resources/settings currently (or even cr_shared_style where we include settings_shared_css). I'm not sure I have an opinion as to whether or not we should change that, but it doesn't seem like we should change it just here.
LGTM https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/user_list.html (right): https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/user_list.html:40: -webkit-margin-start: var(--cr-icon-button-margin-start); On 2017/06/23 at 23:37:46, stevenjb wrote: > On 2017/06/23 23:31:28, dpapad wrote: > > --cr-icon-button-margin-start is defined in shared_vars_css.html. Even if we do > > depend on it transitively from settings_shared_css.html, we should import it > > directly, no (include-what-you-use principle). > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/shared_va... > > We do not currently have any examples of a direct include of shared_vars_css.html in resources/settings currently (or even cr_shared_style where we include settings_shared_css). I'm not sure I have an opinion as to whether or not we should change that, but it doesn't seem like we should change it just here. Ok, I had not realized that was the case. Agreed let's not change the pattern here. I also think that this eventually be affected by the new deprecation warning (tracked by https://bugs.chromium.org/p/chromium/issues/detail?id=735633). settings_vars_css.html will have to change because of it IIUC.
On 2017/06/23 23:49:27, dpapad wrote: > LGTM > > https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/user_list.html (right): > > https://codereview.chromium.org/2959543002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/user_list.html:40: > -webkit-margin-start: var(--cr-icon-button-margin-start); > On 2017/06/23 at 23:37:46, stevenjb wrote: > > On 2017/06/23 23:31:28, dpapad wrote: > > > --cr-icon-button-margin-start is defined in shared_vars_css.html. Even if we > do > > > depend on it transitively from settings_shared_css.html, we should import it > > > directly, no (include-what-you-use principle). > > > > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/shared_va... > > > > We do not currently have any examples of a direct include of > shared_vars_css.html in resources/settings currently (or even cr_shared_style > where we include settings_shared_css). I'm not sure I have an opinion as to > whether or not we should change that, but it doesn't seem like we should change > it just here. > > Ok, I had not realized that was the case. Agreed let's not change the pattern > here. I also think that this eventually be affected by the new deprecation > warning (tracked by > https://bugs.chromium.org/p/chromium/issues/detail?id=735633). > settings_vars_css.html will have to change because of it IIUC. crbug.com/730957 was approved for merge, so committing this.
The CQ bit was checked by stevenjb@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
CLs for remote refs other than refs/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Description was changed from ========== Settings: Users: Fix margins for restrict signins list BUG=730957 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Users: Fix margins for restrict signins list BUG=730957 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true NOPRESUBMIT=true ==========
The CQ bit was checked by stevenjb@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": 1, "attempt_start_ts": 1498266088699900, "parent_rev":
"5560901c7a68077b7c028aaaf4f386863526e8c5", "commit_rev":
"bd0beb1515603576b1e7d12e4351cc5bc4cfd1c8"}
Message was sent while issue was closed.
Description was changed from ========== Settings: Users: Fix margins for restrict signins list BUG=730957 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true NOPRESUBMIT=true ========== to ========== Settings: Users: Fix margins for restrict signins list BUG=730957 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2959543002 Cr-Commit-Position: refs/branch-heads/3112@{#461} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} Committed: https://chromium.googlesource.com/chromium/src/+/bd0beb1515603576b1e7d12e4351... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/bd0beb1515603576b1e7d12e4351... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
