[MD settings] icons for clickable rows

[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or
open-in-new icons to some settings rows. The intent
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

Committed: https://crrev.com/b65e0125470324a339ebf3831d755e68e3d8aea2
Cr-Commit-Position: refs/heads/master@{#421939}

[dschuyler, 2016-09-19 23:49:38]

Description was changed from

==========
[MD settings] add right arrows to clickable rows

This CL adds small right-facing arrow (triangle) icons to some settings
rows. The intent is to give an indication that the row is clickable.

BUG=648457
==========

to

==========
[MD settings] add right arrows to clickable rows

This CL adds small right-facing arrow (triangle) icons to some settings
rows. The intent is to give an indication that the row is clickable.

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

[dschuyler, 2016-09-19 23:49:40]

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

[commit-bot: I haz the power, 2016-09-19 23:50:17]

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

[dschuyler, 2016-09-19 23:52:37]

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

[dschuyler, 2016-09-19 23:52:43]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-19 23:52:45]

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

[dschuyler, 2016-09-19 23:54:42]

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

[dschuyler, 2016-09-19 23:54:42]

This CL touches many files, but it essentially boils
down to adding icons.

In some places .single-column was removed because
the .settings-box is no longer a single column (the
icon is a second column).

In other places a .start class was added to the prior
item (before the icon) to cause a flex expand that 
pushes the icon to the far end.

[dschuyler, 2016-09-19 23:57:36]

On 2016/09/19 23:54:42, dschuyler wrote:
> This CL touches many files, but it essentially boils
> down to adding icons.
> 
> In some places .single-column was removed because
> the .settings-box is no longer a single column (the
> icon is a second column).
> 
> In other places a .start class was added to the prior
> item (before the icon) to cause a flex expand that 
> pushes the icon to the far end.

I added a screen shot to the bug.

[Dan Beam, 2016-09-19 23:57:58]

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

[Dan Beam, 2016-09-19 23:57:58]

hcarmona@: why type of a11y enhancements should these need?

fyi: it was decided that these are uncontroversial enough to start on these >
arrows without UI review.

dschuyler@: why are these <iron-icon> and not <button
is="paper-icon-button-light">?  don't we want them in the tab order / ripply?

[Dan Beam, 2016-09-19 23:58:04]

Description was changed from

==========
[MD settings] add right arrows to clickable rows

This CL adds small right-facing arrow (triangle) icons to some settings
rows. The intent is to give an indication that the row is clickable.

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

to

==========
[MD settings] add right arrows to clickable rows

This CL adds small right-facing arrow (triangle) icons to some settings
rows. The intent is to give an indication that the row is clickable.

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

[Dan Beam, 2016-09-19 23:58:04]

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

[dschuyler, 2016-09-20 00:44:48]

On 2016/09/19 23:57:58, Dan Beam wrote:
> hcarmona@: why type of a11y enhancements should these need?
> 
> fyi: it was decided that these are uncontroversial enough to start on these >
> arrows without UI review.
> 
> dschuyler@: why are these <iron-icon> and not <button
> is="paper-icon-button-light">?  don't we want them in the tab order / ripply?

afaik (and from chatting with Michael), we'd still need the <iron-icon>.
They would get wrapped with paper-icon-button-light or similar to add a11y.

I can add them though :) 

Tommy, would you like to see a bigger CL or two CLs?

[commit-bot: I haz the power, 2016-09-20 01:23:39]

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

[commit-bot: I haz the power, 2016-09-20 01:23:41]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-20 17:44:08]

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

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:21:
</iron-icon>
I hate to do this to you, but is it possible to do this with CSS instead?

i.e. Make a .settings-box.clickable class? 

Perhaps with a background-image or using the CSS :after selector?

[hcarmona, 2016-09-20 21:33:56]

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

[hcarmona, 2016-09-20 21:33:57]

I'm wondering how we're going to handle a clickable row that also has a
toggle? Will the arrow go to the left or the right of the toggle? Is it
weird if the arrow points to another button? Should we use a gear icon
in that case instead?

Do we want to make the entire row focus-able or just the icon?

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:65:
<template is="dom-if" if="[[!expandIsVisible_(deviceState, networkStateList)]]">
WiFi, Cellular, and VPN connections have a gear icon. Maybe we should
show that icon here for ethernet. This will avoid us having 2 buttons
that do the same thing in the case of cellular.

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

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.html:98: <iron-icon
icon="settings:arrow-drop-up" class="settings-arrow-right">
Maybe this button shouldn't have the > because it points to another
button (Sign out). There is also already an icon available for the
avatar.

If we decide to keep it, then it should also be in chromeos.

[Dan Beam, 2016-09-21 04:22:55]

On 2016/09/20 21:33:57, Hector Carmona wrote:
> I'm wondering how we're going to handle a clickable row that also has a
> toggle? Will the arrow go to the left or the right of the toggle? Is it
> weird if the arrow points to another button? Should we use a gear icon
> in that case instead?
> 
> Do we want to make the entire row focus-able or just the icon?

I think the best solution here is to move the toggles into the subpage.  bettes@
wanted that long term.  that way we end up with less stacked components at the
end of each item.

i wonder how much work that'd be.

[dschuyler, 2016-09-22 01:37:36]

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

[commit-bot: I haz the power, 2016-09-22 01:37:52]

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, 2016-09-22 01:40:52]

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

[commit-bot: I haz the power, 2016-09-22 01:40:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dschuyler, 2016-09-22 19:19:25]

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

[commit-bot: I haz the power, 2016-09-22 19:19:53]

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

[dschuyler, 2016-09-22 19:19:55]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-22 20:34:44]

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

[commit-bot: I haz the power, 2016-09-22 20:34:45]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-22 21:04:54]

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

[dschuyler, 2016-09-22 21:05:09]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-22 21:05:25]

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

[dschuyler, 2016-09-22 21:09:01]

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

[dschuyler, 2016-09-22 21:09:17]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-09-22 21:09:26]

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

[dschuyler, 2016-09-22 21:14:11]

Description was changed from

==========
[MD settings] add right arrows to clickable rows

This CL adds small right-facing arrow (triangle) icons to some settings
rows. The intent is to give an indication that the row is clickable.

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

to

==========
[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or 
open-in-new icons to some settings rows. The intent 
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

[commit-bot: I haz the power, 2016-09-22 22:17:25]

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

[commit-bot: I haz the power, 2016-09-22 22:17:26]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-22 23:36:45]

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

[commit-bot: I haz the power, 2016-09-22 23:37:23]

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

[dschuyler, 2016-09-23 00:23:56]

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

[commit-bot: I haz the power, 2016-09-23 00:24:18]

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

[dschuyler, 2016-09-23 00:24:20]

Patchset #2 (id:100001) has been deleted

[dschuyler, 2016-09-23 00:24:28]

Patchset #2 (id:120001) has been deleted

[dschuyler, 2016-09-23 00:31:55]

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

[dschuyler, 2016-09-23 00:32:11]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-23 00:32:34]

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

[dschuyler, 2016-09-23 01:08:09]

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

[commit-bot: I haz the power, 2016-09-23 01:08:23]

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

[dschuyler, 2016-09-23 01:11:46]

Patchset #2 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-09-23 02:05:50]

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

[commit-bot: I haz the power, 2016-09-23 02:05:51]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-23 17:28:50]

On 2016/09/21 04:22:55, Dan Beam wrote:
> On 2016/09/20 21:33:57, Hector Carmona wrote:
> > I'm wondering how we're going to handle a clickable row that also has a
> > toggle? Will the arrow go to the left or the right of the toggle? Is it
> > weird if the arrow points to another button? Should we use a gear icon
> > in that case instead?
> > 
> > Do we want to make the entire row focus-able or just the icon?
> 
> I think the best solution here is to move the toggles into the subpage. 
bettes@
> wanted that long term.  that way we end up with less stacked components at the
> end of each item.
> 
> i wonder how much work that'd be.

It's not the amount of work, it's the UI approval that we're waiting for.

[dschuyler, 2016-09-23 17:29:18]

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:65:
<template is="dom-if" if="[[!expandIsVisible_(deviceState, networkStateList)]]">
On 2016/09/20 21:33:57, Hector Carmona wrote:
> WiFi, Cellular, and VPN connections have a gear icon. Maybe we should
> show that icon here for ethernet. This will avoid us having 2 buttons
> that do the same thing in the case of cellular.

I've decided to keep this CL focused on non-chromeos changes, so I've
removed this file from the CL.

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

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.html:98: <iron-icon
icon="settings:arrow-drop-up" class="settings-arrow-right">
On 2016/09/20 21:33:57, Hector Carmona wrote:
> Maybe this button shouldn't have the > because it points to another
> button (Sign out). There is also already an icon available for the
> avatar.
> 
> If we decide to keep it, then it should also be in chromeos.

The longer term plan is to change the UI further so that the arrows
are not pointing at things to the right. In the short term, we are using
the vertical grey bar to help show that the current box is ending and
the stuff to the right is something else, separate from where the 
arrow is.

I'd like to separate the chromeos changes from this CL because
we are in a development mode where we are prioritizing non-chromeos
desktop work. This CL should not break chromeos, but it may not
polish it as well as non-chromeos.

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

https://codereview.chromium.org/2356653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:21:
</iron-icon>
On 2016/09/20 17:44:08, tommycli wrote:
> I hate to do this to you, but is it possible to do this with CSS instead?
> 
> i.e. Make a .settings-box.clickable class? 
> 
> Perhaps with a background-image or using the CSS :after selector?

The control needs to do more than just be a picture. It's an a11y 
target for example. I was able to leverage your suggestion and 
reduce the number of dom-nodes and shadow-roots (vs. what
it would have been otherwise).

Done.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:30:
</button>
This is another example where the arrow is pointing right
at some other control. It's a toggle button in this case.
Alan plans to have the toggle controls moved into a the
subpages. For this CL, we are using the vertical grey bar
to visually separate the row button from the other control.
(This is an Alan-approved solution while he works on 
getting UI approval for moving the controls).

https://codereview.chromium.org/2356653002/diff/180001/third_party/polymer/v1...
File third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html
(right):

https://codereview.chromium.org/2356653002/diff/180001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html:95:
@apply(--paper-ripple);
A PR has been made for this at
https://github.com/PolymerElements/paper-button/blob/master/paper-button.html

[tommycli, 2016-09-23 19:29:57]

no substantive objections. just nits below. lgtm

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:107: <message
name="IDS_SETTINGS_ACCESSIBILITY_WEB_STORE" desc="In the settings tab, an
explanation that additional accessibility features are found on the Chrome Web
Store.">
nit: update description. "In settings tab," seems odd. I assume this is an
external link?

Maybe something similar to: "Text for an external link explaining that ..."

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:115: </message>
nit: For these 'subtext' labels, I see both fooDescription/FOO_DESCRIPTION as
well as the fooDetails/FOO_DETAILS, as well as the fooSubtitle/FOO_SUBTITLE
convention.

It would be good to choose one of those three rather than adding a fourth. :)

I know we're already in a bad state, we should probably fix it up at some point.

Same comment for all below.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: </button>
nit: something off about indentation here

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File
chrome/browser/resources/settings/default_browser_page/default_browser_page.html
(right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/default_browser_page/default_browser_page.html:42:
</button>
nit: We really should have made one div class and multiple conditionally hidden
spans rather than three div classes.... but that's not really your
responsibility.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:9: --paper-ripple: {
Maybe a comment that these dimenisons are to center over the background image?

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:352:
-webkit-margin-start: 20px;
Why is this required?

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:357: height: 33px;
This seems odd. Why is this height required?

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:96:
{"accessibilityWebStore", IDS_SETTINGS_ACCESSIBILITY_WEB_STORE},
nit: keep same order as settings_strings.grdp? Also wouldn't the JS string (to
be consistent, be a11yWebStore?)

[dschuyler, 2016-09-23 23:59:45]

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

[commit-bot: I haz the power, 2016-09-24 00:00:20]

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

[dschuyler, 2016-09-24 00:15:15]

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

[commit-bot: I haz the power, 2016-09-24 00:15:34]

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

[dschuyler, 2016-09-24 00:20:12]

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

[dschuyler, 2016-09-24 00:20:24]

Patchset #4 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-09-24 00:20:42]

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, 2016-09-24 01:50:13]

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

[commit-bot: I haz the power, 2016-09-24 01:50:14]

Dry run: This issue passed the CQ dry run.

[hcarmona, 2016-09-24 01:55:12]

LGTM w/ a nit

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(left):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:157:
IDS_OPTIONS_SETTINGS_ACCESSIBILITY_ADDITIONAL_FEATURES_DESCRIPTION},
Nit: If this IDS tag is no longer used, should it be removed?

[Dan Beam, 2016-09-24 02:53:58]

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

[Dan Beam, 2016-09-24 02:54:00]

https://codereview.chromium.org/2356653002/diff/240001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:108: Choose from the Chrome Web Store
where did you get all of these strings from?

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:3: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
do you need a paper-icon-button-light import in this file?

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:18:
on-tap="onTapCategory" actionable>
note: you're welcome to change all these to using the [actionable] attribute,
but notice that @apply(--settings-actionable); is already doing this above ^ in
the <style>

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:931:
IDS_SETTINGS_PEOPLE_MANAGE_OTHER_PEOPLE_DESCRIPTION},
why are these shuffling?

https://codereview.chromium.org/2356653002/diff/240001/third_party/polymer/v1...
File third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html
(right):

https://codereview.chromium.org/2356653002/diff/240001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html:95:
@apply(--paper-ripple);
i'm confused.  anything that is in :host {...} can be overridden from the
outside.  why do you need this?  are you worried that we wont apply the styles
in every shadow DOM?

[dschuyler, 2016-09-27 00:43:10]

This is just replying to some of the review questions. I'm not intending to get
this re-reviewed.

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:107: <message
name="IDS_SETTINGS_ACCESSIBILITY_WEB_STORE" desc="In the settings tab, an
explanation that additional accessibility features are found on the Chrome Web
Store.">
On 2016/09/23 19:29:56, tommycli wrote:
> nit: update description. "In settings tab," seems odd. I assume this is an
> external link?
> 
> Maybe something similar to: "Text for an external link explaining that ..."

Done.

https://codereview.chromium.org/2356653002/diff/180001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:115: </message>
On 2016/09/23 19:29:56, tommycli wrote:
> nit: For these 'subtext' labels, I see both fooDescription/FOO_DESCRIPTION as
> well as the fooDetails/FOO_DETAILS, as well as the fooSubtitle/FOO_SUBTITLE
> convention.
> 
> It would be good to choose one of those three rather than adding a fourth. :)
> 
> I know we're already in a bad state, we should probably fix it up at some
point.
> 
> Same comment for all below.

Done.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: </button>
On 2016/09/23 19:29:57, tommycli wrote:
> nit: something off about indentation here

Done.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File
chrome/browser/resources/settings/default_browser_page/default_browser_page.html
(right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/default_browser_page/default_browser_page.html:42:
</button>
On 2016/09/23 19:29:57, tommycli wrote:
> nit: We really should have made one div class and multiple conditionally
hidden
> spans rather than three div classes.... but that's not really your
> responsibility.

Acknowledged.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:9: --paper-ripple: {
On 2016/09/23 19:29:57, tommycli wrote:
> Maybe a comment that these dimenisons are to center over the background image?

Done.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:352:
-webkit-margin-start: 20px;
On 2016/09/23 19:29:57, tommycli wrote:
> Why is this required?

I know it looks strange because normally we use 
a margin x-or padding. In this case the vertical
line is made by the border of the div that wraps
the contents in the secondary-action. The margin 
is on one side of the border and the padding is 
on the other side.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:357: height: 33px;
On 2016/09/23 19:29:57, tommycli wrote:
> This seems odd. Why is this height required?

It previously didn't have a height and the 
height was implicit from the contents of 
secondary-action. That height is 33px. That
just happened to work out until I put something
other than a button in the secondary-action.
So now it's being set explicitly so that it's
consistent.

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2356653002/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:96:
{"accessibilityWebStore", IDS_SETTINGS_ACCESSIBILITY_WEB_STORE},
On 2016/09/23 19:29:57, tommycli wrote:
> nit: keep same order as settings_strings.grdp? Also wouldn't the JS string (to
> be consistent, be a11yWebStore?)

Done.

https://codereview.chromium.org/2356653002/diff/240001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:108: Choose from the Chrome Web Store
On 2016/09/24 02:53:59, Dan Beam wrote:
> where did you get all of these strings from?

bettes@ or prior version.

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:931:
IDS_SETTINGS_PEOPLE_MANAGE_OTHER_PEOPLE_DESCRIPTION},
On 2016/09/24 02:53:59, Dan Beam wrote:
> why are these shuffling?

They are mostly new strings.

https://codereview.chromium.org/2356653002/diff/240001/third_party/polymer/v1...
File third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html
(right):

https://codereview.chromium.org/2356653002/diff/240001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html:95:
@apply(--paper-ripple);
On 2016/09/24 02:53:59, Dan Beam wrote:
> i'm confused.  anything that is in :host {...} can be overridden from the
> outside.  

That's true if you're right outside the item. Like being the host of
paper-ripple.

> why do you need this?  are you worried that we wont apply the styles
> in every shadow DOM?

in the paper-icon-button-light, the paper-ripple is embedded so I don't know how
to access it to change it (without doing bad things). I considered adding a
mixin to paper-icon-button-light, but then reasoned that it would be better
(useful in more contexts) to have it in paper-ripple.

[Dan Beam, 2016-09-27 23:03:54]

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: </button>
can we continue with these

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/system_page/system_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/system_page/system_page.html:49:
<controlled-button pref="[[prefs.proxy]]">
but not these?

[dschuyler, 2016-09-28 20:49:07]

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

[commit-bot: I haz the power, 2016-09-28 20:49:43]

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

[dschuyler, 2016-09-28 20:53:09]

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

[commit-bot: I haz the power, 2016-09-28 20:53:32]

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

[dschuyler, 2016-09-28 20:53:45]

Patchset #5 (id:260001) has been deleted

[dschuyler, 2016-09-28 20:56:54]

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

[commit-bot: I haz the power, 2016-09-28 20:57:10]

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

[dschuyler, 2016-09-28 20:58:30]

Patchset #5 (id:280001) has been deleted

[dschuyler, 2016-09-28 21:01:04]

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

[dschuyler, 2016-09-28 21:01:21]

Patchset #5 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-09-28 21:01:28]

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

[dschuyler, 2016-09-28 21:03:26]

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:145: </button>
On 2016/09/27 23:03:53, Dan Beam wrote:
> can we continue with these

Done.

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:3: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
On 2016/09/24 02:53:59, Dan Beam wrote:
> do you need a paper-icon-button-light import in this file?

Done.

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:18:
on-tap="onTapCategory" actionable>
On 2016/09/24 02:53:59, Dan Beam wrote:
> note: you're welcome to change all these to using the [actionable] attribute,
> but notice that @apply(--settings-actionable); is already doing this above ^
in
> the <style>

I'm using [actionable] in css, so I removed the 
@apply(--settings-actionable); above.

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/system_page/system_page.html (right):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/system_page/system_page.html:49:
<controlled-button pref="[[prefs.proxy]]">
On 2016/09/27 23:03:53, Dan Beam wrote:
> but not these?

I kept the ones that are not a controlled-button.
(As we talked about in person).

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(left):

https://codereview.chromium.org/2356653002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:157:
IDS_OPTIONS_SETTINGS_ACCESSIBILITY_ADDITIONAL_FEATURES_DESCRIPTION},
On 2016/09/24 01:55:12, Hector Carmona wrote:
> Nit: If this IDS tag is no longer used, should it be removed?

Done.

[commit-bot: I haz the power, 2016-09-28 21:56:25]

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

[commit-bot: I haz the power, 2016-09-28 21:56:26]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-09-28 21:56:27]

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:163: </button>
many of these fit on one line, and they probably should be

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:24:
-webkit-margin-end: var(--iron-icon-spacing);
why are you removing this?

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:46:
<iron-icon icon="settings:palette"></iron-icon>
are we dropping this now or later?

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:24: background-image:
url("data:image/svg+xml;charset=UTF-8,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%2224px%22%20height%3D%2224px%22%20viewBox%3D%220%200%2024%2024%22%20fill%3D%22%23757575%22%3E%3Cg%3E%3Cpath%20d%3D%22M7%2014l5-5%205%205z%22%3E%3C%2Fpath%3E%3C%2Fg%3E%3C%2Fsvg%3E");
can you just put this in a file and use url(local-path.svg) and add
flattenhtml="true" in the .grd file that pulls settings_shared_css.html into
chrome?

this dynamically data URL-ifies the path for us (so we don't have this in the
source code)

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:362: height: 33px;
what is this height for?

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:13:
</style>
nit: put on one line

<style include="settings-shared"></style>

https://codereview.chromium.org/2356653002/diff/320001/third_party/polymer/v1...
File third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html:95:
@apply(--paper-ripple);
I think it'd still be better to reduce the scope of this by styling from

third_party/polymer/components/paper-icon-button/paper-icon-button-light.html

which has access to the paper-ripple it houses, i.e. 

in paper-icon-button-light.html:

<style>

paper-ripple {
  /* ... */
  @apply(--paper-icon-button-light-ripple);
}

</style>

and then from settings_shared_css.html we set:

--paper-icon-button-light-ripple: {
  /* contents of what is currently in "--paper-ripple" */
};

I'm fairly worried that people would overwrite eachother with --paper-ripple (as
they're like everywhere) and processing that many many times (with all our
ripples) might be inefficient.

[commit-bot: I haz the power, 2016-09-28 21:56:49]

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

[commit-bot: I haz the power, 2016-09-28 21:56:50]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-28 23:58:54]

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

[commit-bot: I haz the power, 2016-09-28 23:59:19]

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

[dschuyler, 2016-09-29 00:30:44]

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

[commit-bot: I haz the power, 2016-09-29 00:31:04]

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

[dschuyler, 2016-09-29 00:31:19]

Patchset #6 (id:340001) has been deleted

[dschuyler, 2016-09-29 00:39:15]

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

[commit-bot: I haz the power, 2016-09-29 00:39:37]

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

[dschuyler, 2016-09-29 00:40:57]

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:163: </button>
On 2016/09/28 21:56:26, Dan Beam wrote:
> many of these fit on one line, and they probably should be

Done.

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:24:
-webkit-margin-end: var(--iron-icon-spacing);
On 2016/09/28 21:56:27, Dan Beam wrote:
> why are you removing this?

It's not needed. Is it needed and I'm missing
where that is?

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:46:
<iron-icon icon="settings:palette"></iron-icon>
On 2016/09/28 21:56:26, Dan Beam wrote:
> are we dropping this now or later?

I'd like to avoiding adding more to this CL.

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:24: background-image:
url("data:image/svg+xml;charset=UTF-8,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%2224px%22%20height%3D%2224px%22%20viewBox%3D%220%200%2024%2024%22%20fill%3D%22%23757575%22%3E%3Cg%3E%3Cpath%20d%3D%22M7%2014l5-5%205%205z%22%3E%3C%2Fpath%3E%3C%2Fg%3E%3C%2Fsvg%3E");
On 2016/09/28 21:56:27, Dan Beam wrote:
> can you just put this in a file and use url(local-path.svg) and add
> flattenhtml="true" in the .grd file that pulls settings_shared_css.html into
> chrome?
> 
> this dynamically data URL-ifies the path for us (so we don't have this in the
> source code)

Done.

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:362: height: 33px;
On 2016/09/28 21:56:27, Dan Beam wrote:
> what is this height for?

It was the highest size used for this and 
I was making it consistent. In hindsight
it's better to use the row height.

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:13:
</style>
On 2016/09/28 21:56:27, Dan Beam wrote:
> nit: put on one line
> 
> <style include="settings-shared"></style>

Done.

https://codereview.chromium.org/2356653002/diff/320001/third_party/polymer/v1...
File third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple.html:95:
@apply(--paper-ripple);
On 2016/09/28 21:56:27, Dan Beam wrote:
> I think it'd still be better to reduce the scope of this by styling from
> 
> third_party/polymer/components/paper-icon-button/paper-icon-button-light.html
> 
> which has access to the paper-ripple it houses, i.e. 
> 
> in paper-icon-button-light.html:
> 
> <style>
> 
> paper-ripple {
>   /* ... */
>   @apply(--paper-icon-button-light-ripple);
> }
> 
> </style>
> 
> and then from settings_shared_css.html we set:
> 
> --paper-icon-button-light-ripple: {
>   /* contents of what is currently in "--paper-ripple" */
> };
> 
> I'm fairly worried that people would overwrite eachother with --paper-ripple
(as
> they're like everywhere) and processing that many many times (with all our
> ripples) might be inefficient.

Done.

https://codereview.chromium.org/2356653002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2356653002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:33: </button>
These are just barely too long when wrapped.

[commit-bot: I haz the power, 2016-09-29 02:34:03]

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

[commit-bot: I haz the power, 2016-09-29 02:34:04]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-09-29 03:19:34]

lgtm w/nits

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:24:
-webkit-margin-end: var(--iron-icon-spacing);
On 2016/09/29 00:40:57, dschuyler wrote:
> On 2016/09/28 21:56:27, Dan Beam wrote:
> > why are you removing this?
> 
> It's not needed. Is it needed and I'm missing
> where that is?

so you're saying this rule didn't have any affect?  it looks like it's matching
<iron-icon> tags inside of class="settings-box" in this file

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:29:
<iron-icon icon="settings:wallpaper"></iron-icon>
like this

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:8:
button[is="paper-icon-button-light"] {
i'm pretty sure there's a CSS presubmit about using single quotes instead of
double quotes (which the rest of the file does).  can we do that?

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:24: background-image:
url("images/arrow_right.svg");
url(images/arrow_right.svg);

(no quotes)

[dschuyler, 2016-09-29 18:35:50]

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

[commit-bot: I haz the power, 2016-09-29 18:36:13]

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, 2016-09-29 21:00:36]

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

[commit-bot: I haz the power, 2016-09-29 21:00:38]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-29 21:22:50]

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:24:
-webkit-margin-end: var(--iron-icon-spacing);
On 2016/09/29 03:19:34, Dan Beam wrote:
> On 2016/09/29 00:40:57, dschuyler wrote:
> > On 2016/09/28 21:56:27, Dan Beam wrote:
> > > why are you removing this?
> > 
> > It's not needed. Is it needed and I'm missing
> > where that is?
> 
> so you're saying this rule didn't have any affect?  it looks like it's
matching
> <iron-icon> tags inside of class="settings-box" in this file

I see where that would look wrong. There was an earlier
mistake in that a .start class was used after the iron-icon
rather than a .middle class. The .middle has space before it.
So the icon doesn't need space after it. This change removes
a minor local hack of having a one-off custom css spacing.

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:29:
<iron-icon icon="settings:wallpaper"></iron-icon>
On 2016/09/29 03:19:34, Dan Beam wrote:
> like this

Acknowledged.

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:8:
button[is="paper-icon-button-light"] {
On 2016/09/29 03:19:34, Dan Beam wrote:
> i'm pretty sure there's a CSS presubmit about using single quotes instead of
> double quotes (which the rest of the file does).  can we do that?

Done.

https://codereview.chromium.org/2356653002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:24: background-image:
url("images/arrow_right.svg");
On 2016/09/29 03:19:34, Dan Beam wrote:
> url(images/arrow_right.svg);
> 
> (no quotes)

Done.

[dschuyler, 2016-09-29 21:23:02]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-09-29 21:23:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, hcarmona@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2356653002/#ps400001
(title: "review nits")

[commit-bot: I haz the power, 2016-09-29 21:23:20]

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, 2016-09-29 21:29:38]

Description was changed from

==========
[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or 
open-in-new icons to some settings rows. The intent 
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

to

==========
[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or 
open-in-new icons to some settings rows. The intent 
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

[commit-bot: I haz the power, 2016-09-29 21:29:40]

Committed patchset #8 (id:400001)

[commit-bot: I haz the power, 2016-09-29 21:31:56]

Description was changed from

==========
[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or 
open-in-new icons to some settings rows. The intent 
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

to

==========
[MD settings] icons for clickable rows

This CL adds small right-facing arrow (triangle) or
open-in-new icons to some settings rows. The intent
is to give an indication that the row is clickable.
This CL focuses on the non-chromeos desktops. A few
chromeos specific settings will be updated
separately.

There is a small change to paper-ripple to add a
mixin.

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

Committed: https://crrev.com/b65e0125470324a339ebf3831d755e68e3d8aea2
Cr-Commit-Position: refs/heads/master@{#421939}
==========

[commit-bot: I haz the power, 2016-09-29 21:31:58]

Patchset 8 (id:??) landed as
https://crrev.com/b65e0125470324a339ebf3831d755e68e3d8aea2
Cr-Commit-Position: refs/heads/master@{#421939}

[Dan Beam, 2016-09-29 21:45:45]

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/2356653002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.html:24:
-webkit-margin-end: var(--iron-icon-spacing);
On 2016/09/29 21:22:50, dschuyler wrote:
> On 2016/09/29 03:19:34, Dan Beam wrote:
> > On 2016/09/29 00:40:57, dschuyler wrote:
> > > On 2016/09/28 21:56:27, Dan Beam wrote:
> > > > why are you removing this?
> > > 
> > > It's not needed. Is it needed and I'm missing
> > > where that is?
> > 
> > so you're saying this rule didn't have any affect?  it looks like it's
> matching
> > <iron-icon> tags inside of class="settings-box" in this file
> 
> I see where that would look wrong. There was an earlier
> mistake in that a .start class was used after the iron-icon
> rather than a .middle class. The .middle has space before it.
> So the icon doesn't need space after it. This change removes
> a minor local hack of having a one-off custom css spacing.

i see now, thanks for 'splainin