[MD settings] adding settings-row class

[MD settings] adding settings-row class

'settings-row' shows a row of controls for a setting.  This is used as
a simple container style for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
Committed: https://crrev.com/808be52137d2f1d070140d1885e3994a6e4fb2f9
Cr-Commit-Position: refs/heads/master@{#357897}

[dschuyler, 2015-10-20 17:54:09]

Description was changed from

==========
[MD settings] adding settings-row element

BUG=425627
==========

to

==========
[MD settings] adding settings-row element

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a <left>,
a <right> or both.  Elements withing left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

[dschuyler, 2015-10-20 17:54:09]

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

[stevenjb, 2015-10-20 18:19:21]

https://codereview.chromium.org/1409373007/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_row.css (right):

https://codereview.chromium.org/1409373007/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_row.css:9: padding: 8px
16px 8px 16px;
nit: I think this can be 'padding: 8px 16px'. Not actually sure if that is
preferred.

[stevenjb, 2015-10-20 18:26:49]

I would suggest putting this in c/b/r/settings/controls.

[Dan Beam, 2015-10-20 18:31:50]

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

[Dan Beam, 2015-10-20 18:31:51]

why is this better than a class?

https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_row.js (right):

https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_row.js:16: *     <left>
custom tags should have-dashes

[Dan Beam, 2015-10-20 18:32:04]

(as in, .left, .right, .row)

[dschuyler, 2015-10-20 20:46:50]

On 2015/10/20 18:31:51, Dan Beam wrote:
> why is this better than a class?

I'll change it to a set of classes.

(I thought maybe an element could swap the left and right in rtl languages;
and an element looked more future-proof).

> 
>
https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_page/settings_row.js (right):
> 
>
https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_page/settings_row.js:16: *    
<left>
> custom tags should have-dashes

[dschuyler, 2015-10-20 20:46:59]

https://codereview.chromium.org/1409373007/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_row.css (right):

https://codereview.chromium.org/1409373007/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_row.css:9: padding: 8px
16px 8px 16px;
On 2015/10/20 18:19:21, stevenjb wrote:
> nit: I think this can be 'padding: 8px 16px'. Not actually sure if that is
> preferred.

We may need to update the padding in the future.  This is intended to get row
wrappers all through the settings - then the padding and such can be adjusted to
match the final UI spec.

Done.

https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_row.js (right):

https://codereview.chromium.org/1409373007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_row.js:16: *     <left>
On 2015/10/20 18:31:51, Dan Beam wrote:
> custom tags should have-dashes

Ah, I knew about dashes for polymer 
module names - thanks for the tip about
the dashes for all custom tags.

Maybe left-side, right-side

or

Maybe a custom property?  i.e. 
<settings-row>
  <div left>
...

[dschuyler, 2015-10-20 22:37:02]

I've removed the element and added a class.

[stevenjb, 2015-10-20 23:34:30]

Nice. lgtm.

[michaelpg, 2015-10-20 23:39:00]

michaelpg@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2015-10-20 23:39:00]

I already styled these rows using ".item-list paper-item" to provide padding,
borders, etc. Can you de-dupe those styles with .settings-row?

[dschuyler, 2015-10-23 21:35:11]

On 2015/10/20 23:39:00, michaelpg wrote:
> I already styled these rows using ".item-list paper-item" to provide padding,
> borders, etc. Can you de-dupe those styles with .settings-row?

I've added CL 1418553005 to follow this CL with those change.

[dschuyler, 2015-10-28 19:21:00]

On 2015/10/23 21:35:11, dschuyler wrote:
> On 2015/10/20 23:39:00, michaelpg wrote:
> > I already styled these rows using ".item-list paper-item" to provide
padding,
> > borders, etc. Can you de-dupe those styles with .settings-row?
> 
> I've added CL 1418553005 to follow this CL with those change.

Michael, I've added the height and icon padding here (that 
you'd asked about in CL 1418553005).

[michaelpg, 2015-10-28 20:35:29]

lgtm

https://codereview.chromium.org/1409373007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1409373007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:91: 
nit: remove extra newline

[dschuyler, 2015-10-28 20:54:09]

https://codereview.chromium.org/1409373007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1409373007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_page.css:91: 
On 2015/10/28 20:35:29, michaelpg wrote:
> nit: remove extra newline

Done.

[Dan Beam, 2015-10-28 20:55:56]

please update the CL description/title

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_page.css:78: height:
40px;  /* 24px + 2x8px padding. */
does this assume we never have multi-line content?

[Dan Beam, 2015-10-28 20:56:42]

also "Elements withing" -> within

[dschuyler, 2015-10-28 21:02:08]

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_page.css:78: height:
40px;  /* 24px + 2x8px padding. */
On 2015/10/28 20:55:56, Dan Beam wrote:
> does this assume we never have multi-line content?

Michael, should this be min-height instead of height?
And should it be 24px (is the padding handled by the padding: value below)?

[dschuyler, 2015-10-28 21:05:17]

Description was changed from

==========
[MD settings] adding settings-row element

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a <left>,
a <right> or both.  Elements withing left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

to

==========
[MD settings] adding settings-row element

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

[michaelpg, 2015-10-31 21:31:45]

Sorry, I never sent this draft!

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_page.css (right):

https://codereview.chromium.org/1409373007/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_page.css:78: height:
40px;  /* 24px + 2x8px padding. */
On 2015/10/28 21:02:07, dschuyler wrote:
> On 2015/10/28 20:55:56, Dan Beam wrote:
> > does this assume we never have multi-line content?
> 
> Michael, should this be min-height instead of height?
> And should it be 24px (is the padding handled by the padding: value below)?

yeah, I guess min-height should be 20px (the specs now show 10px top and bottom
padding). The spec conflicts with itself though so let's not spend too much time
on it -- if it looks good, ship it :)

[dschuyler, 2015-11-02 19:04:45]

updated with min-height.  PTAL

[dschuyler, 2015-11-04 18:57:00]

On 2015/11/02 19:04:45, dschuyler wrote:
> updated with min-height.  PTAL

ping

[Dan Beam, 2015-11-04 19:07:24]

On 2015/10/28 20:55:56, Dan Beam wrote:
> please update the CL description/title

pong

[dschuyler, 2015-11-04 19:10:09]

Description was changed from

==========
[MD settings] adding settings-row element

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

to

==========
[MD settings] adding settings-row class

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

[dschuyler, 2015-11-04 19:10:57]

Description was changed from

==========
[MD settings] adding settings-row class

'settings-row' shows a row of controls for a setting.  This is used as
a simple container for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

to

==========
[MD settings] adding settings-row class

'settings-row' shows a row of controls for a setting.  This is used as
a simple container style for UI group that is smaller than a page or section,
but larger than a single button or slider.  The row may contain a left part,
a right part or both.  Elements within left or right will be pushed to the
respective ends of the row.

BUG=425627
==========

[dschuyler, 2015-11-04 19:14:42]

On 2015/11/04 19:07:24, Dan Beam wrote:
> On 2015/10/28 20:55:56, Dan Beam wrote:
> > please update the CL description/title
> 
> pong

I've now also changed element to class (in addition to the prior change in the
description).

[Dan Beam, 2015-11-04 19:55:58]

lgtm

[dschuyler, 2015-11-04 20:16:18]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-11-04 20:16:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1409373007/#ps120001
(title: "changed height to min-height 20px")

[commit-bot: I haz the power, 2015-11-04 20:17:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409373007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409373007/120001

[commit-bot: I haz the power, 2015-11-04 21:04:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-04 21:06:20]

Patchset 7 (id:??) landed as
https://crrev.com/808be52137d2f1d070140d1885e3994a6e4fb2f9
Cr-Commit-Position: refs/heads/master@{#357897}