Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893483002:
[MD settings] layout vertical separators with paper-icon-button-light (Closed)
|
Created:
3 years, 7 months ago by dschuyler Modified:
3 years, 7 months ago Reviewers:
stevenjb CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] layout vertical separators with paper-icon-button-light
This CL adjusts the margins and padding on vertical grey line separators
and paper-icon-button-light buttons.
BUG=723146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2893483002
Cr-Commit-Position: refs/heads/master@{#472976}
Committed: https://chromium.googlesource.com/chromium/src/+/8a14c40573ecd9fbf68ac4c4d97b9e4c9f504a6d
Patch Set 1 #
Total comments: 11
Patch Set 2 : comment #
Total comments: 2
Patch Set 3 : comment change #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. BUG= ========== to ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. 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...
Description was changed from ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. BUG=723146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm a little cautious about our use of negative margins here, so I just want to double check that we have checked everything carefully and considered alternatives. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/network/cr_network_list_item.html:50: -webkit-padding-start: 20px; That is a fair bit of padding. I assume you checked this pretty thoroughly, including with a chromeos=1 build? (Ignoring the places in Networking where we shouldn't have a separator at all of course). https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:30: -webkit-margin-start: calc(var(--cr-icon-ripple-padding) * -1); So, just to be clear, we are changing this from a start padding of 16px to -8px? We should add a comment about overlapping the start as well here.
https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/network/cr_network_list_item.html:50: -webkit-padding-start: 20px; On 2017/05/17 16:39:37, stevenjb wrote: > That is a fair bit of padding. I assume you checked this pretty thoroughly, > including with a chromeos=1 build? (Ignoring the places in Networking where we > shouldn't have a separator at all of course). I did a chromeos build if that's what you mean. (target_os = "chromeos" in args.gn). This is also only used in this one file. I don't think iron-a11y-keys has UI, so there's really just the <button> to consider - is that correct? https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:30: -webkit-margin-start: calc(var(--cr-icon-ripple-padding) * -1); On 2017/05/17 16:39:37, stevenjb wrote: > So, just to be clear, we are changing this from a start padding of 16px to -8px? True. > We should add a comment about overlapping the start as well here. What comment would be good here - the negative values may be a clear indication that the item is shifting to some readers. Maybe: - <no comment, just remove it> - /* Shift the button toward the end of the row. */ - /* Allow ripple to overlap the start and the end. */ This last one reads weirdly to me (harder for me to figure out what the comment is trying to say than figure out what the code is doing).
lgtm assuming the wifi networks and known networks lists look OK. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/network/cr_network_list_item.html:50: -webkit-padding-start: 20px; On 2017/05/17 21:28:14, dschuyler wrote: > On 2017/05/17 16:39:37, stevenjb wrote: > > That is a fair bit of padding. I assume you checked this pretty thoroughly, > > including with a chromeos=1 build? (Ignoring the places in Networking where we > > shouldn't have a separator at all of course). > > I did a chromeos build if that's what you mean. (target_os = "chromeos" in > args.gn). > This is also only used in this one file. I don't think iron-a11y-keys has UI, so > there's really just the <button> to consider - is that correct? Yes, if the list of wifi networks (click on the wifi row in the main page) still looks correct, and the list of known networks (click on 'known networks' in the wifi page) both look correct, then this should be fine. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:30: -webkit-margin-start: calc(var(--cr-icon-ripple-padding) * -1); On 2017/05/17 21:28:14, dschuyler wrote: > On 2017/05/17 16:39:37, stevenjb wrote: > > So, just to be clear, we are changing this from a start padding of 16px to > -8px? > > True. > > > We should add a comment about overlapping the start as well here. > > What comment would be good here - the negative values may be a clear indication > that the item is shifting to some readers. Maybe: > > - <no comment, just remove it> > > - /* Shift the button toward the end of the row. */ > > - /* Allow ripple to overlap the start and the end. */ > This last one reads weirdly to me (harder for me to figure > out what the comment is trying to say than figure out > what the code is doing). How about just: /* Allow the ripple to overlap the icon. */ To me, the comment as it currently is suggests that we are just overlapping the end, but now we are overlapping both.
https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/network/cr_network_list_item.html:49: -webkit-border-start: var(--cr-separator-line); Do you want to go ahead and just remove this while you are looking at this? (For crbug.com/723150) https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:27: --cr-paper-icon-button-margin: { I also noticed that this is used by cr-expand-button, I assume this still looks correct (or looks more correct now)?
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...
I've added screen shots in the bug that show a sampling of affected buttons, ptal. I took another shot at that comment too. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/network/cr_network_list_item.html:49: -webkit-border-start: var(--cr-separator-line); On 2017/05/17 22:00:49, stevenjb wrote: > Do you want to go ahead and just remove this while you are looking at this? (For > crbug.com/723150) Sure, I'll look at that, but I'd like to do it separately. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:27: --cr-paper-icon-button-margin: { On 2017/05/17 22:00:49, stevenjb wrote: > I also noticed that this is used by cr-expand-button, I assume this still looks > correct (or looks more correct now)? added screen shots to help verify. https://codereview.chromium.org/2893483002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_vars_css.html:30: -webkit-margin-start: calc(var(--cr-icon-ripple-padding) * -1); On 2017/05/17 21:36:04, stevenjb wrote: > On 2017/05/17 21:28:14, dschuyler wrote: > > On 2017/05/17 16:39:37, stevenjb wrote: > > > So, just to be clear, we are changing this from a start padding of 16px to > > -8px? > > > > True. > > > > > We should add a comment about overlapping the start as well here. > > > > What comment would be good here - the negative values may be a clear > indication > > that the item is shifting to some readers. Maybe: > > > > - <no comment, just remove it> > > > > - /* Shift the button toward the end of the row. */ > > > > - /* Allow ripple to overlap the start and the end. */ > > This last one reads weirdly to me (harder for me to figure > > out what the comment is trying to say than figure out > > what the code is doing). > > How about just: > > /* Allow the ripple to overlap the icon. */ > > To me, the comment as it currently is suggests that we are just overlapping the > end, but now we are overlapping both. :), that kinda sounds like the ripple is overlapping the icon, which is normal. I took another shot at the comment, wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2893483002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_vars_css.html:28: /* Move button so ripple overlaps the end of the row. */ nit: Shift?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
https://codereview.chromium.org/2893483002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/shared_vars_css.html (right): https://codereview.chromium.org/2893483002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/shared_vars_css.html:28: /* Move button so ripple overlaps the end of the row. */ On 2017/05/18 18:57:44, stevenjb wrote: > nit: Shift? Done.
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.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2893483002/#ps40001 (title: "comment change")
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": 40001, "attempt_start_ts": 1495150727005160,
"parent_rev": "385931fbf1990470dde91e5f12c75db5680193c4", "commit_rev":
"8a14c40573ecd9fbf68ac4c4d97b9e4c9f504a6d"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. BUG=723146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] layout vertical separators with paper-icon-button-light This CL adjusts the margins and padding on vertical grey line separators and paper-icon-button-light buttons. BUG=723146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2893483002 Cr-Commit-Position: refs/heads/master@{#472976} Committed: https://chromium.googlesource.com/chromium/src/+/8a14c40573ecd9fbf68ac4c4d97b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8a14c40573ecd9fbf68ac4c4d97b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
