[MD settings] drawer menu icon into title bar; icon colors

[MD settings] drawer menu icon into title bar; icon colors

This CL fixes the side nav drawer menu icon to appear within
the title bar.  This also adds the internal search input and
icon (though doesn't implement internal search).  Other icon
color fixes are made in the side nav and site settings cookie
icons.

BUG=599657, 598892, 597862
Committed: https://crrev.com/c80526b8ba29a72abe4bee22d1351a1798dd87e0
Cr-Commit-Position: refs/heads/master@{#387651}

[dschuyler, 2016-04-13 00:16:30]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-04-13 18:21:22]

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

[tommycli, 2016-04-13 20:39:29]

https://codereview.chromium.org/1880063004/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1880063004/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:115: <message
name="IDS_SETTINGS_INTERNAL_SEARCH" desc="Label for search within settings input
field.">
nit / perhaps: desc="Label for input field to search within Settings"

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:30:
--settings-title-search-color: rgb(192,199,205);
spacing between commas

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:28: :root {
Do these changes apply only to the paper-header-panels? Can they be under
paper-header-panel instead of :root then? So that these variables are only
defined for that scope?

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
Are there RTL issues here?

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:75: margin-left:
8px;
RTL?

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.js:44: onNarrow_:
function(e) {
Since it's not always narrowed (sometimes it's expanded), can we name this
something else?

[dschuyler, 2016-04-13 22:19:06]

Patchset #2 (id:40001) has been deleted

[dschuyler, 2016-04-13 22:19:51]

https://codereview.chromium.org/1880063004/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1880063004/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:115: <message
name="IDS_SETTINGS_INTERNAL_SEARCH" desc="Label for search within settings input
field.">
On 2016/04/13 20:39:29, tommycli wrote:
> nit / perhaps: desc="Label for input field to search within Settings"

Done.

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:30:
--settings-title-search-color: rgb(192,199,205);
On 2016/04/13 20:39:29, tommycli wrote:
> spacing between commas

Done.

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:28: :root {
On 2016/04/13 20:39:29, tommycli wrote:
> Do these changes apply only to the paper-header-panels? Can they be under
> paper-header-panel instead of :root then? So that these variables are only
> defined for that scope?

Done.

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
On 2016/04/13 20:39:29, tommycli wrote:
> Are there RTL issues here?

This is undoing a specific entry (margin-right) within
the paper-toolbar. I think there is an RTL issue, but it's
lower level (in polymer) than this code.

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:75: margin-left:
8px;
On 2016/04/13 20:39:29, tommycli wrote:
> RTL?

Done.

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.js:44: onNarrow_:
function(e) {
On 2016/04/13 20:39:29, tommycli wrote:
> Since it's not always narrowed (sometimes it's expanded), can we name this
> something else?

Done.

[tommycli, 2016-04-13 22:35:29]

Apologies for more commentary, but i thought of one potentially even faster way
to accomplish the hiding.

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
On 2016/04/13 22:19:50, dschuyler wrote:
> On 2016/04/13 20:39:29, tommycli wrote:
> > Are there RTL issues here?
> 
> This is undoing a specific entry (margin-right) within
> the paper-toolbar. I think there is an RTL issue, but it's
> lower level (in polymer) than this code.

Okay cool. I see that now. Maybe just add a TODO or Polymer bug to verify that
either: a) it is indeed always on the right in both regular and RTL environments
or b) it's a Polymer RTL bug and we need to address it.

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

https://codereview.chromium.org/1880063004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:109: <div
class="heading" hidden$="[[!isNarrow_]]">
Instead of a data binding, does a CSS rule like this work?

paper-drawer-panel[narrow] #heading {
  display: none
}
?

https://codereview.chromium.org/1880063004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:114:
active="{{isSearchActive_}}"></paper-icon-button>
Same here, can we use a CSS rule instead like...

#searchToggleButton:not([active]) {
  display: none;
}

[Dan Beam, 2016-04-13 22:51:14]

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:60: line-height:
16px !important;
don't use !important if at all possible

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
On 2016/04/13 22:35:28, tommycli wrote:
> On 2016/04/13 22:19:50, dschuyler wrote:
> > On 2016/04/13 20:39:29, tommycli wrote:
> > > Are there RTL issues here?
> > 
> > This is undoing a specific entry (margin-right) within
> > the paper-toolbar. I think there is an RTL issue, but it's
> > lower level (in polymer) than this code.
> 
> Okay cool. I see that now. Maybe just add a TODO or Polymer bug to verify that
> either: a) it is indeed always on the right in both regular and RTL
environments
> or b) it's a Polymer RTL bug and we need to address it.

use -webkit-margin-start/end in a more specific way or
:host-context([dir='rtl']) and make this mirror (also run the code with
LANGUAGE=ar or something from command line to actually see settings in RTL)

[dschuyler, 2016-04-14 20:44:38]

Patchset #3 (id:80001) has been deleted

[dschuyler, 2016-04-14 21:06:12]

Patchset #3 (id:100001) has been deleted

[dschuyler, 2016-04-14 21:25:00]

Patchset #3 (id:120001) has been deleted

[dschuyler, 2016-04-14 21:27:22]

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

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:60: line-height:
16px !important;
On 2016/04/13 22:51:14, Dan Beam (ooo) wrote:
> don't use !important if at all possible

Done.

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
On 2016/04/13 22:35:28, tommycli wrote:
> On 2016/04/13 22:19:50, dschuyler wrote:
> > On 2016/04/13 20:39:29, tommycli wrote:
> > > Are there RTL issues here?
> > 
> > This is undoing a specific entry (margin-right) within
> > the paper-toolbar. I think there is an RTL issue, but it's
> > lower level (in polymer) than this code.
> 
> Okay cool. I see that now. Maybe just add a TODO or Polymer bug to verify that
> either: a) it is indeed always on the right in both regular and RTL
environments
> or b) it's a Polymer RTL bug and we need to address it.

Done.

https://codereview.chromium.org/1880063004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:71: margin-right:
0;
On 2016/04/13 22:51:14, Dan Beam (ooo) wrote:
> On 2016/04/13 22:35:28, tommycli wrote:
> > On 2016/04/13 22:19:50, dschuyler wrote:
> > > On 2016/04/13 20:39:29, tommycli wrote:
> > > > Are there RTL issues here?
> > > 
> > > This is undoing a specific entry (margin-right) within
> > > the paper-toolbar. I think there is an RTL issue, but it's
> > > lower level (in polymer) than this code.
> > 
> > Okay cool. I see that now. Maybe just add a TODO or Polymer bug to verify
that
> > either: a) it is indeed always on the right in both regular and RTL
> environments
> > or b) it's a Polymer RTL bug and we need to address it.
> 
> use -webkit-margin-start/end in a more specific way or
> :host-context([dir='rtl']) and make this mirror (also run the code with
> LANGUAGE=ar or something from command line to actually see settings in RTL)

Acknowledged.

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

https://codereview.chromium.org/1880063004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:109: <div
class="heading" hidden$="[[!isNarrow_]]">
On 2016/04/13 22:35:28, tommycli wrote:
> Instead of a data binding, does a CSS rule like this work?
> 
> paper-drawer-panel[narrow] #heading {
>   display: none
> }
> ?

Done.

https://codereview.chromium.org/1880063004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:114:
active="{{isSearchActive_}}"></paper-icon-button>
On 2016/04/13 22:35:28, tommycli wrote:
> Same here, can we use a CSS rule instead like...
> 
> #searchToggleButton:not([active]) {
>   display: none;
> }

Done.

[tommycli, 2016-04-15 00:15:15]

lgtm sans:

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:60: /*
nit: \n needed before this line

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:103:
paper-drawer-panel:not([narrow]) #main-title {
Wow, this is cool! This will make it super fast.

[dschuyler, 2016-04-15 00:49:15]

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui.html (right):

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:60: /*
On 2016/04/15 00:15:15, tommycli wrote:
> nit: \n needed before this line

Done.

https://codereview.chromium.org/1880063004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui.html:103:
paper-drawer-panel:not([narrow]) #main-title {
On 2016/04/15 00:15:15, tommycli wrote:
> Wow, this is cool! This will make it super fast.

Acknowledged.

[dschuyler, 2016-04-15 18:21:36]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-04-15 18:21:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1880063004/#ps160001
(title: "added blank line")

[commit-bot: I haz the power, 2016-04-15 18:22:16]

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

[commit-bot: I haz the power, 2016-04-15 18:27:03]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-04-15 18:28:57]

Description was changed from

==========
[MD settings] drawer menu icon into title bar; icon colors

This CL fixes the side nav drawer menu icon to appear within
the title bar.  This also adds the internal search input and
icon (though doesn't implement internal search).  Other icon
color fixes are made in the side nav and site settings cookie
icons.

BUG=599657, 598892, 597862
==========

to

==========
[MD settings] drawer menu icon into title bar; icon colors

This CL fixes the side nav drawer menu icon to appear within
the title bar.  This also adds the internal search input and
icon (though doesn't implement internal search).  Other icon
color fixes are made in the side nav and site settings cookie
icons.

BUG=599657, 598892, 597862

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

[commit-bot: I haz the power, 2016-04-15 18:28:58]

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