[MD settings] css for row separator

[MD settings] css for row separator

This CL adds CSS for .separator which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2847583002
Cr-Commit-Position: refs/heads/master@{#468471}
Committed: https://chromium.googlesource.com/chromium/src/+/898ae07dedccc8e9a9a125c662ce1c28af55d41f

[dschuyler, 2017-04-26 22:58:17]

Description was changed from

==========
[MD settings] css for vertical-rule-line

This CL adds CSS for vertical-rule-line which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
==========

to

==========
[MD settings] css for vertical-rule-line

This CL adds CSS for vertical-rule-line which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-04-26 22:58:20]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 22:59:00]

Dry run: CQ is trying da patch.

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

[dschuyler, 2017-04-26 23:02:49]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[dschuyler, 2017-04-26 23:03:39]

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

[dschuyler, 2017-04-26 23:03:40]

screen shots at http://imgur.com/a/Rq5Aa

[commit-bot: I haz the power, 2017-04-26 23:04:06]

Dry run: CQ is trying da patch.

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

[Dan Beam, 2017-04-27 00:11:03]

in an ideal world, separator would harness height: calc(100% - <offset>).  but
i've been having problems doing that when the parent is display: flex; and i'm
lazy.

in a slightly less ideal world, each control that's explicitly sized (i.e.
.settings-box) might have a --min-row-height that is simply changed across
selectors, as in:

:root {
  --settings-row-min-height: 45px;
  --separator-gap: 5px;
}

.setting-box {
  min-height: var(--settings-row-min-height);
}

.settings-box.two-line {
  --settings-row-min-height: 64px;
}

and then the separator would have a variable to calculate it's absolute height
assuming it's embedded reliably:

.settings-box .separator {
  height: calc(var(--settings-row-min-height) -
               2 * var(--separator-gap));
  top: var(--separator-gap);
}

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:321: /*
TODO(dschuyler): replace with .vertical-rule-line.
wait, why can't we do this now?

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:352:
.vertical-rule-line {
nit: i like .separator more because it's not so tied to the presentation

[dschuyler, 2017-04-27 00:15:24]

On 2017/04/27 00:11:03, Dan Beam wrote:
> in an ideal world, separator would harness height: calc(100% - <offset>).  but

I gave that some thought and came back to 80% because if the line were equal to
<offset> the separator would have no height. Using a straight percentage allows
for some visible part of the separator and still have a spacing above/below.
That seems more idea - what am I missing?

> i've been having problems doing that when the parent is display: flex; and i'm
> lazy.
> 
> in a slightly less ideal world, each control that's explicitly sized (i.e.
> .settings-box) might have a --min-row-height that is simply changed across
> selectors, as in:
> 
> :root {
>   --settings-row-min-height: 45px;
>   --separator-gap: 5px;
> }
> 
> .setting-box {
>   min-height: var(--settings-row-min-height);
> }
> 
> .settings-box.two-line {
>   --settings-row-min-height: 64px;
> }
> 
> and then the separator would have a variable to calculate it's absolute height
> assuming it's embedded reliably:
> 
> .settings-box .separator {
>   height: calc(var(--settings-row-min-height) -
>                2 * var(--separator-gap));
>   top: var(--separator-gap);
> }
> 
>
https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_shared_css.html (right):
> 
>
https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_shared_css.html:321: /*
> TODO(dschuyler): replace with .vertical-rule-line.
> wait, why can't we do this now?
> 
>
https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_shared_css.html:352:
> .vertical-rule-line {
> nit: i like .separator more because it's not so tied to the presentation

[Dan Beam, 2017-04-27 00:20:06]

On 2017/04/27 00:15:24, dschuyler wrote:
> On 2017/04/27 00:11:03, Dan Beam wrote:
> > in an ideal world, separator would harness height: calc(100% - <offset>). 
but
> 
> I gave that some thought and came back to 80% because if the line were equal
to
> <offset> the separator would have no height. Using a straight percentage
allows
> for some visible part of the separator and still have a spacing above/below.
> That seems more idea - what am I missing?

i think that if we guarantee min-heights in the proper range this is not an
issue.

also, here's a prototype of what i was talking about with vars:
https://jsfiddle.net/y0o8y2cL/2/

[dschuyler, 2017-04-27 00:36:14]

On 2017/04/27 00:20:06, Dan Beam wrote:
> On 2017/04/27 00:15:24, dschuyler wrote:
> > On 2017/04/27 00:11:03, Dan Beam wrote:
> > > in an ideal world, separator would harness height: calc(100% - <offset>). 
> but
> > 
> > I gave that some thought and came back to 80% because if the line were equal
> to
> > <offset> the separator would have no height. Using a straight percentage
> allows
> > for some visible part of the separator and still have a spacing above/below.
> > That seems more idea - what am I missing?
> 
> i think that if we guarantee min-heights in the proper range this is not an
> issue.
> 
> also, here's a prototype of what i was talking about with vars:
> https://jsfiddle.net/y0o8y2cL/2/

Thanks. In the example, the two line row should have a 46px height rather than
54px.
Please also consider (large) font sizes and how the vertical line scales.

[commit-bot: I haz the power, 2017-04-27 00:50:38]

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

[commit-bot: I haz the power, 2017-04-27 00:50:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-04-27 01:10:42]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 01:11:33]

Dry run: CQ is trying da patch.

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

[dschuyler, 2017-04-27 01:13:12]

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:321: /*
TODO(dschuyler): replace with .vertical-rule-line.
On 2017/04/27 00:11:03, Dan Beam wrote:
> wait, why can't we do this now?

I was looking to hammer out the details before spreading it too far. e.g. name
changes. I also wanted to present an example of what I was suggesting to be sure
it was acceptable.

https://codereview.chromium.org/2847583002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:352:
.vertical-rule-line {
On 2017/04/27 00:11:03, Dan Beam wrote:
> nit: i like .separator more because it's not so tied to the presentation

Done.

[dschuyler, 2017-04-27 01:23:02]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 01:23:33]

Dry run: 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-04-27 03:26:20]

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

[commit-bot: I haz the power, 2017-04-27 03:26:21]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-04-27 18:25:21]

Description was changed from

==========
[MD settings] css for vertical-rule-line

This CL adds CSS for vertical-rule-line which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] css for row separator

This CL adds CSS for .separator which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-04-29 02:58:13]

so, are we still using this or did you move this to:
https://codereview.chromium.org/2849663002/
?

[Dan Beam, 2017-05-01 15:32:31]

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:87:
id="themesSecondarActions">
secondary

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:349: /* The
vertical-rule-line is a separator line like a horizontal rule
can you update this comment?

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:355:
-webkit-padding-start: var(--settings-box-row-padding);
is there a reason we have to use padding instead of margin-end?

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:357:
--settings-separator-gaps: 9px;
so wait, does this mean 4.5px on top and bottom? do we want an even number?

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:364: 2 *
var(--settings-separator-gaps));
why is this doubled in this case?

https://codereview.chromium.org/2847583002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:184:
assertTrue(appearancePage.$$('#themesSecondarActions').hidden);
at least you're consistent...

[dschuyler, 2017-05-01 19:21:45]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:22:10]

Dry run: CQ is trying da patch.

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

[dschuyler, 2017-05-01 19:25:10]

On 2017/04/29 02:58:13, Dan Beam wrote:
> so, are we still using this or did you move this to:
> https://codereview.chromium.org/2849663002/
> ?

That CL goes through the remaining files to replace secondary-action with
separator. The intent was to keep this CL smaller and more focused. This CL is
still important.

[dschuyler, 2017-05-01 19:26:11]

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:87:
id="themesSecondarActions">
On 2017/05/01 15:32:31, Dan Beam wrote:
> secondary

Done.

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:349: /* The
vertical-rule-line is a separator line like a horizontal rule
On 2017/05/01 15:32:31, Dan Beam wrote:
> can you update this comment?

Done.

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:355:
-webkit-padding-start: var(--settings-box-row-padding);
On 2017/05/01 15:32:31, Dan Beam wrote:
> is there a reason we have to use padding instead of margin-end?

There used to be (when the div had contents), but that no longer applies.

Done.

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:357:
--settings-separator-gaps: 9px;
On 2017/05/01 15:32:31, Dan Beam wrote:
> so wait, does this mean 4.5px on top and bottom? do we want an even number?

The numbers bettes@ asked for is 45px row with a 36px vertical line. It looks
good in practice because the half px allows for an anit-aliasing tapering of the
tips. I'd question Alan further on it if it looked wrong, but it looks fine imo.

https://codereview.chromium.org/2847583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:364: 2 *
var(--settings-separator-gaps));
On 2017/05/01 15:32:31, Dan Beam wrote:
> why is this doubled in this case?

This is from an aesthetic call by UI (bettes@): on a 64px row the grey line
should be 56px. This could also be done with a separate variable like
--settings-two-line-separator-gaps: 18px.

https://codereview.chromium.org/2847583002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2847583002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:184:
assertTrue(appearancePage.$$('#themesSecondarActions').hidden);
On 2017/05/01 15:32:31, Dan Beam wrote:
> at least you're consistent...

Done.

[Dan Beam, 2017-05-01 19:35:18]

lgtm

[commit-bot: I haz the power, 2017-05-01 21:41:19]

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

[commit-bot: I haz the power, 2017-05-01 21:41:20]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-05-01 22:56:42]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-05-01 22:57:32]

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-05-01 23:04:12]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493679402802930,
"parent_rev": "dc843fac8da1a46453dd6035c910c07f5237922c", "commit_rev":
"898ae07dedccc8e9a9a125c662ce1c28af55d41f"}

[commit-bot: I haz the power, 2017-05-01 23:04:24]

Description was changed from

==========
[MD settings] css for row separator

This CL adds CSS for .separator which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] css for row separator

This CL adds CSS for .separator which is a grey separator line
between controls on a row (e.g. settings-box). The intent is to replace
.secondary-action (a TODO has been added to that effect). This initial
CL changes the separator lines seen in the basic main page of MD
settings.

(a step to fix, but not complete fix).
BUG=714320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2847583002
Cr-Commit-Position: refs/heads/master@{#468471}
Committed:
https://chromium.googlesource.com/chromium/src/+/898ae07dedccc8e9a9a125c662ce...
==========

[commit-bot: I haz the power, 2017-05-01 23:04:26]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/898ae07dedccc8e9a9a125c662ce...