MD Settings: Replace current toolbar with cr-toolbar.

MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add string for  search placeholder.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet (displayed along the placeholder text).

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/48c8c979b1440101b879a05352cfd5907a8ef643
Cr-Commit-Position: refs/heads/master@{#401192}

[dpapad, 2016-06-14 00:28:45]

Description was changed from

==========
MD Settings: Replace paper-toolbar with cr-toolbar.

BUG=608535
==========

to

==========
MD Settings: Replace paper-toolbar with cr-toolbar.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-14 23:31:34]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-06-14 23:31:38]

Patchset #1 (id:20001) has been deleted

[dpapad, 2016-06-14 23:31:42]

Patchset #1 (id:40001) has been deleted

[dpapad, 2016-06-14 23:42:26]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org, michaelpg@chromium.org

[dpapad, 2016-06-14 23:42:27]

This CL heavily changes the UI of the toolbar, see after screenshots at
http://imgur.com/a/1s5rt.

@michaelpg: A lot of animation related CSS code seems unnecessary with this CL
and is being removed. I performed sanity check locally to ensure that animations
run correctly, but please verify.

Also I am still facing a minor issue where I can't change the color of the
search and 'x' icons, but I prefer not to block this CL on it.

[michaelpg, 2016-06-15 18:01:19]

well, lgtm w/ nits, but I'm totally unfamiliar with the toolbar CSS so I may not
be the best reviewer.

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

https://codereview.chromium.org/2064013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:43: transition:
background-color 300ms, color 300ms, height 300ms;
nit: line breaks

https://codereview.chromium.org/2064013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_ui/settings_ui.html:70:
@apply(--layout-center);
nit: put @apply at top

[dschuyler, 2016-06-15 20:39:26]

Just making a note here that we've been chatting about
comparing cr-toolbar with app-toolbar and paper elements.

[dschuyler, 2016-06-15 20:39:28]

Just making a note here that we've been chatting about
comparing cr-toolbar with app-toolbar and paper elements.

[dpapad, 2016-06-20 19:11:37]

On 2016/06/15 at 20:39:28, dschuyler wrote:
> Just making a note here that we've been chatting about
> comparing cr-toolbar with app-toolbar and paper elements.

Please  hold off reviewing this CL further. I am going to wait until the
transition to app-drawer (https://codereview.chromium.org/2074063003) has
landed, and until the menu button has been added to the cr-toolbar, before
rebasing this CL.
Thank you

[dpapad, 2016-06-20 21:38:54]

Description was changed from

==========
MD Settings: Replace paper-toolbar with cr-toolbar.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Replace current toolbar with cr-toolbar.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-20 21:40:10]

Description was changed from

==========
MD Settings: Replace current toolbar with cr-toolbar.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add necessary strings for placeholder and 'x' button tooltip.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-20 21:40:46]

Description was changed from

==========
MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add necessary strings for placeholder and 'x' button tooltip.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet.

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add string for  search placeholder.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet (displayed along the placeholder text).

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-06-20 22:07:28]

dpapad@chromium.org changed reviewers:
+ tsergeant@chromium.org

[dpapad, 2016-06-20 22:07:29]

This CL has been fully reworked, and it now depends on the app-drawer CL
(https://codereview.chromium.org/2074063003).

Screenshots of how cr-toolbar looks within MD Settings are at
http://imgur.com/a/Vmlzf.

Per offline discussion with @michaelpg, I am adding a temporary "coming soon!"
text within the search box to make it clear that searching is not implemented
yet.

@tsergeant: See cr_toolbar changes. It adds a way to display a menu icon next to
the page title. If MD History also wants this icon, we could remove the
"<content select=..>" and directly put the icon in cr-toolbar, but I am leaving
this for a future CL.

Finally, given that MD Settings is always in narrow mode, we might want to add a
way to force the cr-toolbar to always be in narrow mode. I am not doing this in
this CL, since it is not clear to me that we want this yet. I'll follow up
though.

[dschuyler, 2016-06-20 23:38:00]

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

https://codereview.chromium.org/2064013002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:31:
#include "ash/system/chromeos/devicetype_utils.h"
Did this change or is this a whoops?
For me, this file is in ash/common/,
but maybe my code is out of date?

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:43: <content
select="paper-icon-button"></content>
Can we go with a more specific selector, such 
as [drawer-button] or [drawer-toggle]?

e.g.

https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

[dpapad, 2016-06-20 23:55:07]

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

https://codereview.chromium.org/2064013002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:31:
#include "ash/system/chromeos/devicetype_utils.h"
On 2016/06/20 at 23:37:59, dschuyler wrote:
> Did this change or is this a whoops?
> For me, this file is in ash/common/,
> but maybe my code is out of date?

Reverted. This was accidental.

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:43: <content
select="paper-icon-button"></content>
On 2016/06/20 at 23:37:59, dschuyler wrote:
> Can we go with a more specific selector, such 
> as [drawer-button] or [drawer-toggle]?
> 
> e.g.
> 
>
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

From the cr-toolbar's point of view, there is no assumption that this button is
linked to a drawer/toggle, this is just a button, and clients can potentially
use it for different actions, so I am a bit reluctant encapsulating a specific
usage in the name.

[tsergeant, 2016-06-21 00:04:30]

Yay! I'm glad this mostly Just Worked.

cr_toolbar lgtm

> @tsergeant: See cr_toolbar changes. It adds a way to display a menu icon next
to
> the page title. If MD History also wants this icon, we could remove the
> "<content select=..>" and directly put the icon in cr-toolbar, but I am
leaving
> this for a future CL.

I'm currently adding the same thing to MD History, so I'm happy to make this
follow-up change.

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):

https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:21:
-webkit-padding-start: 24px;
Eyeballing your screenshots, it looks like there's slightly too much padding to
the left now. I'm guessing the paper-icon-button adds an extra 6-8px of padding?

This might be a bit nasty to fix without moving the paper-icon-button into the
cr-toolbar, so I'm happy to defer it to a follow-up. Feel free to add a TODO
assigned to me.

[dpapad, 2016-06-21 00:30:47]

On 2016/06/21 at 00:04:30, tsergeant wrote:
> Yay! I'm glad this mostly Just Worked.

Yup, seems to be working pretty well so far!

> 
> cr_toolbar lgtm
> 
> > @tsergeant: See cr_toolbar changes. It adds a way to display a menu icon
next to
> > the page title. If MD History also wants this icon, we could remove the
> > "<content select=..>" and directly put the icon in cr-toolbar, but I am
leaving
> > this for a future CL.
> 
> I'm currently adding the same thing to MD History, so I'm happy to make this
follow-up change.

Thanks! If this CL is blocking you (still waiting for other reviewers to take a
look), feel free
to perform the cr-toolbar change, and I can just rebase once your CL lands.

> 
>
https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
> File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right):
> 
>
https://codereview.chromium.org/2064013002/diff/100001/ui/webui/resources/cr_...
> ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:21:
-webkit-padding-start: 24px;
> Eyeballing your screenshots, it looks like there's slightly too much padding
to the left now. I'm guessing the paper-icon-button adds an extra 6-8px of
padding?
> 
> This might be a bit nasty to fix without moving the paper-icon-button into the
cr-toolbar, so I'm happy to defer it to a follow-up. Feel free to add a TODO
assigned to me.

You are right, there seems to be a bit more padding. I will add a TODO if this
CL lands before your changes.

[dschuyler, 2016-06-21 01:33:17]

LGTM based on tsergeant@ comments about a follow-up CL.

[dpapad, 2016-06-21 18:14:56]

@michaelpg: Can you verify that this is still LG? You had LGTMed prior to
comment15, and the CL was reworked quite a bit at that point.

[michaelpg, 2016-06-21 23:58:03]

On 2016/06/21 at 18:14:56, dpapad wrote:
> @michaelpg: Can you verify that this is still LG? You had LGTMed prior to
comment15, and the CL was reworked quite a bit at that point.

slgtm

[dpapad, 2016-06-22 00:18:06]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-06-22 00:18:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2064013002/#ps120001
(title: "Undo accidental change.")

[commit-bot: I haz the power, 2016-06-22 00:19:05]

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

[commit-bot: I haz the power, 2016-06-22 03:26:07]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-06-22 03:27:06]

Description was changed from

==========
MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add string for  search placeholder.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet (displayed along the placeholder text).

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Replace current toolbar with cr-toolbar.

 - Modify cr-toolbar to accept an icon next to the page title.
 - Add string for  search placeholder.
 - Add temporary "coming soon!" text to indicate that search is not
   functional yet (displayed along the placeholder text).

BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/48c8c979b1440101b879a05352cfd5907a8ef643
Cr-Commit-Position: refs/heads/master@{#401192}
==========

[commit-bot: I haz the power, 2016-06-22 03:27:07]

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