MD Settings: Fix network list styling

MD Settings: Fix network list styling

This affects the MD Settings Internet page and MD OOBE

BUG=701945
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2754753002
Cr-Commit-Position: refs/heads/master@{#459527}
Committed: https://chromium.googlesource.com/chromium/src/+/d997c2a566ad1193bc7a2119a2734cdc6631303d

[stevenjb, 2017-03-16 02:05:09]

Description was changed from

==========
MD Settings: Fix network list styling

This affects the MD Settings Internet page and MD OOBE

BUG=701945
==========

to

==========
MD Settings: Fix network list styling

This affects the MD Settings Internet page and MD OOBE

BUG=701945
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-03-16 02:06:40]

stevenjb@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dschuyler, 2017-03-16 19:05:56]

May I see a screen shot of the change?

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:21: min-height:
48px;
Can these be the same as --settings-box-min-height and maybe make a
--cr-row-min-height: 45px and use that in both places (the settings- version
would be removed)?
Or if this needs to be 48px instead of 45px maybe have two vars.
Or if a larger value can be used, maybe the two line height can be used?

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:26:
-webkit-margin-start: 20px;
I think I'm averse to hardcoded px values that are not vars :)
This is defined in settings as --settings-box-row-padding.
Maybe a --cr-row-horizontal-padding: 20px could be use here and replace
--settings-box-row-padding?
The goal being that if there is a design change to make it 22px or something
that it change everywhere easily. (The row height, for example has changed a
couple times).

[stevenjb, 2017-03-16 19:31:51]

On 2017/03/16 19:05:56, dschuyler wrote:
> May I see a screen shot of the change?
> 
>
https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/network/cr_network_list.html (right):
> 
>
https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/network/cr_network_list.html:21: min-height:
> 48px;
> Can these be the same as --settings-box-min-height and maybe make a
> --cr-row-min-height: 45px and use that in both places (the settings- version
> would be removed)?
> Or if this needs to be 48px instead of 45px maybe have two vars.
> Or if a larger value can be used, maybe the two line height can be used?
> 
>
https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):
> 
>
https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/network/cr_network_list_item.html:26:
> -webkit-margin-start: 20px;
> I think I'm averse to hardcoded px values that are not vars :)
> This is defined in settings as --settings-box-row-padding.
> Maybe a --cr-row-horizontal-padding: 20px could be use here and replace
> --settings-box-row-padding?
> The goal being that if there is a design change to make it 22px or something
> that it change everywhere easily. (The row height, for example has changed a
> couple times).

Screenshot: chrome://md-settings/networks?type=WiFi

[stevenjb, 2017-03-16 19:53:52]

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:21: min-height:
48px;
On 2017/03/16 19:05:56, dschuyler wrote:
> Can these be the same as --settings-box-min-height and maybe make a
> --cr-row-min-height: 45px and use that in both places (the settings- version
> would be removed)?
> Or if this needs to be 48px instead of 45px maybe have two vars.
> Or if a larger value can be used, maybe the two line height can be used?

I hate to add a var to the shared vars css just for one use in cr_elements.

The min height really just needs to be > 1 to ensure the iron-list stamps, but I
figured it may as well match an item height.

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:26:
-webkit-margin-start: 20px;
On 2017/03/16 19:05:56, dschuyler wrote:
> I think I'm averse to hardcoded px values that are not vars :)
> This is defined in settings as --settings-box-row-padding.
> Maybe a --cr-row-horizontal-padding: 20px could be use here and replace
> --settings-box-row-padding?
> The goal being that if there is a design change to make it 22px or something
> that it change everywhere easily. (The row height, for example has changed a
> couple times).

Again, same issue. Thus shared UI happens to match settings UI, but no other
shared component shares this padding. Yes there is risk of them getting out of
sync, but there is also some cost to adding a var shared by everything. Also, if
we change values in settings we need to consider whether we want or not we want
this change to affect OOBE which also uses this.

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:63: padding: 0
var(--cr-control-spacing);
Whereas we use this in a couple of places in cr_elements so I went ahead and
added it (and removed cr-icon-padding).

[stevenjb, 2017-03-17 00:03:10]

I also found a regression in the ripple which was getting clipped, so added some
additional changes to address that. (I tried to find a more elegant solution but
failed; I am open to suggestions).

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:21: min-height:
48px;
On 2017/03/16 19:53:51, stevenjb wrote:
> On 2017/03/16 19:05:56, dschuyler wrote:
> > Can these be the same as --settings-box-min-height and maybe make a
> > --cr-row-min-height: 45px and use that in both places (the settings- version
> > would be removed)?
> > Or if this needs to be 48px instead of 45px maybe have two vars.
> > Or if a larger value can be used, maybe the two line height can be used?
> 
> I hate to add a var to the shared vars css just for one use in cr_elements.
> 
> The min height really just needs to be > 1 to ensure the iron-list stamps, but
I
> figured it may as well match an item height.
> 

Actually, I'm pretty certain we don't actually need min-height any more;
removed.

[dschuyler, 2017-03-18 01:22:42]

lgtm

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2754753002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:26:
-webkit-margin-start: 20px;
On 2017/03/16 19:53:51, stevenjb wrote:
> On 2017/03/16 19:05:56, dschuyler wrote:
> > I think I'm averse to hardcoded px values that are not vars :)
> > This is defined in settings as --settings-box-row-padding.
> > Maybe a --cr-row-horizontal-padding: 20px could be use here and replace
> > --settings-box-row-padding?
> > The goal being that if there is a design change to make it 22px or something
> > that it change everywhere easily. (The row height, for example has changed a
> > couple times).
> 
> Again, same issue. Thus shared UI happens to match settings UI, but no other
> shared component shares this padding. Yes there is risk of them getting out of
> sync, but there is also some cost to adding a var shared by everything. Also,
if
> we change values in settings we need to consider whether we want or not we
want
> this change to affect OOBE which also uses this.

I disagree, but it's not worth holding up the CL (here and elsewhere size
constants are used). 

Please consider adding a comment in the code about why the constant is here.

(I or someone like-minded, would see this and want to make a const of it -- if
that's not desired, then a comment would help prevent me (or my doppelganger)
from changing this on ya).

[stevenjb, 2017-03-24 17:57:41]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-24 17:57:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2754753002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-24 17:58:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 18:56:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 18:56:56]

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_...)

[stevenjb, 2017-03-24 19:22:36]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-03-24 19:23:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 19:59:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490383356355220,
"parent_rev": "96fb832e2f8abff293fb34fd320e2222707f585b", "commit_rev":
"d997c2a566ad1193bc7a2119a2734cdc6631303d"}

[commit-bot: I haz the power, 2017-03-24 19:59:54]

Description was changed from

==========
MD Settings: Fix network list styling

This affects the MD Settings Internet page and MD OOBE

BUG=701945
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix network list styling

This affects the MD Settings Internet page and MD OOBE

BUG=701945
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2754753002
Cr-Commit-Position: refs/heads/master@{#459527}
Committed:
https://chromium.googlesource.com/chromium/src/+/d997c2a566ad1193bc7a2119a273...
==========

[commit-bot: I haz the power, 2017-03-24 19:59:57]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d997c2a566ad1193bc7a2119a273...