MD Settings: Manage search engines, polish to match latest mocks.

MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles in search engines list.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.
 - Move vertical dots icon closer to the edge.

BUG=479359
Committed: https://crrev.com/358f1077057fc264978b7bb02d3e35f39421ffab
Cr-Commit-Position: refs/heads/master@{#377204}

[dpapad, 2016-02-19 23:34:54]

Description was changed from

==========
MD Settings: Manage search engines, polish to match latest mocks.

BUG=479359
==========

to

==========
MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.

BUG=479359
==========

[dpapad, 2016-02-19 23:37:24]

Description was changed from

==========
MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.

BUG=479359
==========

to

==========
MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles in search engines list.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.
 - Move vertical dots icon closer to the edge.

BUG=479359
==========

[dpapad, 2016-02-20 02:09:51]

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

[dpapad, 2016-02-20 02:09:52]

Before/after screenshots at http://imgur.com/a/d17Af.

[dpapad, 2016-02-23 00:03:08]

On 2016/02/20 at 02:09:52, dpapad wrote:
> Before/after screenshots at http://imgur.com/a/d17Af.

Friendly ping.

[Dan Beam, 2016-02-23 02:45:12]

looking pretty good

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

https://codereview.chromium.org/1713283002/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:474: <message
name="IDS_SETTINGS_SEARCH_ENGINES_DEFAULT_ENGINES" desc="Label for 'default'
Search Engines section">
"Title of 'default' Search Engines section" sounds more useful to me, but I see
that you're just following protocol

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:28:
text-align: left;
RTL?

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:32:
flex: 3.5;
why are you using .5 here and previously?

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:22:
<template is="dom-if" if="[[engine.iconURL]]">
indent off

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:26:
<iron-icon icon="icons:find-in-page"></iron-icon>
nit: why not

  <iron-icon icon="icons:find-in-page" src="[[engine.iconURL]]">
  </iron-icon>

in the iron-icon doc it says [src] wins over [icon] if it's present

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:42:
isDefault_: function() {
nit: computeIsDefault_

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:5:
<link rel="import" type="css" href="search_engines_list.css">
why are you adding a new <style> tag?  can't you put this code in ^
search_engines_list.css?

[dpapad, 2016-02-23 21:27:04]

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

https://codereview.chromium.org/1713283002/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:474: <message
name="IDS_SETTINGS_SEARCH_ENGINES_DEFAULT_ENGINES" desc="Label for 'default'
Search Engines section">
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> "Title of 'default' Search Engines section" sounds more useful to me, but I
see that you're just following protocol

Ack.

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:28:
text-align: left;
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> RTL?

Removed, this ended up being not needed. There are other things need to be
addressed for RTL though (some of them could be in Polymer), so I suggest doing
the remaining stuff in separate CL as needed.

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:32:
flex: 3.5;
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> why are you using .5 here and previously?

For two reasons,
1) It seems to work in the dev console.
2) It is much more convenient when adjusting visually the widths, to experiment
with a decimal value compared to keeping all values as integers and having to
adjust all children of the flexbox parent for every experimentation.

Are decimal values not allowed? I thought when using flexbox everything is
interpreted as a percentage of the maximum assigned, (for example 5 and 2.5, is
effectively same as 2 and 1).

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:22:
<template is="dom-if" if="[[engine.iconURL]]">
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:26:
<iron-icon icon="icons:find-in-page"></iron-icon>
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> nit: why not
> 
>   <iron-icon icon="icons:find-in-page" src="[[engine.iconURL]]">
>   </iron-icon>
> 
> in the iron-icon doc it says [src] wins over [icon] if it's present

Tried it and it does not work (only the default icon is displayed for all
entries). Even if that part worked though, I don't think that the
icon="someIcon" would take over src=<empty string>, an empty icon would be
displayed for such cases.

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:42:
isDefault_: function() {
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> nit: computeIsDefault_

Done.

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:5:
<link rel="import" type="css" href="search_engines_list.css">
On 2016/02/23 at 02:45:12, Dan Beam wrote:
> why are you adding a new <style> tag?  can't you put this code in ^
search_engines_list.css?

Done, moved to the existing CSS file.

[Dan Beam, 2016-02-23 22:08:38]

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:32:
flex: 3.5;
On 2016/02/23 21:27:04, dpapad wrote:
> On 2016/02/23 at 02:45:12, Dan Beam wrote:
> > why are you using .5 here and previously?
> 
> For two reasons,
> 1) It seems to work in the dev console.
> 2) It is much more convenient when adjusting visually the widths, to
experiment
> with a decimal value compared to keeping all values as integers and having to
> adjust all children of the flexbox parent for every experimentation.
> 
> Are decimal values not allowed? I thought when using flexbox everything is
> interpreted as a percentage of the maximum assigned, (for example 5 and 2.5,
is
> effectively same as 2 and 1).

right, but where are these widths coming from?  bettes?

[dpapad, 2016-02-23 22:14:08]

On 2016/02/23 at 22:08:38, dbeam wrote:
>
https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
> File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):
> 
>
https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:32:
flex: 3.5;
> On 2016/02/23 21:27:04, dpapad wrote:
> > On 2016/02/23 at 02:45:12, Dan Beam wrote:
> > > why are you using .5 here and previously?
> > 
> > For two reasons,
> > 1) It seems to work in the dev console.
> > 2) It is much more convenient when adjusting visually the widths, to
experiment
> > with a decimal value compared to keeping all values as integers and having
to
> > adjust all children of the flexbox parent for every experimentation.
> > 
> > Are decimal values not allowed? I thought when using flexbox everything is
> > interpreted as a percentage of the maximum assigned, (for example 5 and 2.5,
is
> > effectively same as 2 and 1).
> 
> right, but where are these widths coming from?  bettes?

Sort of (explaining below). When I met with Alan to talk about various UI issues
we specified the values of those columns but apparently we messed up the numbers
a bit. Pasting below from
https://groups.google.com/a/google.com/d/msg/chrome-settings/xdak1bSgcTU/3pJK...,

"when total content width is 622px, the columns should be as follows (but
expressed in percentages)
Search engine + icon: 392px + 12px right margin.
Keyword: 320px + 12px right margin.
URL: 324px + 20px right margin."

I say we messed them up because if you add 392+12+320+12+324+20 it does not add
up to 622px. So for now I am simply deducing these values myself by trying to
match the mocks as close as possibly. Once I get a hold on Alan we can refine
those values if they are not good enough.

[Dan Beam, 2016-02-23 22:19:47]

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:27:
flex: 0.5;
why is the icon flexing at all?  that doesn't make much sense

[dpapad, 2016-02-23 22:40:25]

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.css:27:
flex: 0.5;
On 2016/02/23 at 22:19:47, Dan Beam wrote:
> why is the icon flexing at all?  that doesn't make much sense

Removed. I was using display:flex in name-column, and then I was assigning a
flex value in .icon-container and .name, but you are right, this was not needed.

[Dan Beam, 2016-02-23 23:11:15]

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:29:
<span class="name">[[engine.displayName]]</span>
wait, does this change to a computed display: inline; if you move from <div> ->
<span>?  why do we want that and were is it overridden otherwise?  does it not
matter if it's flexing?

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.css
(right):

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.css:20:
margin: 0 40px;
indent off

[dpapad, 2016-02-23 23:25:54]

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:29:
<span class="name">[[engine.displayName]]</span>
On 2016/02/23 23:11:15, Dan Beam wrote:
> wait, does this change to a computed display: inline; if you move from <div>
->
> <span>?  why do we want that and were is it overridden otherwise?  does it not
> matter if it's flexing?

The "name-column" div is not having display:flex anymore (per previous comment).
That means that its children ("icon-container" and "name") follow normal rules
of display block (div) vs inline (span). So in order to have the icon and the
name appear side by side I need to change to a span instead of a div.

Does this address your question? (I am not sure I fully understood it)

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.css
(right):

https://codereview.chromium.org/1713283002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.css:20:
margin: 0 40px;
On 2016/02/23 23:11:15, Dan Beam wrote:
> indent off

Done.

[Dan Beam, 2016-02-23 23:59:13]

lgtm

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:26:
<iron-icon icon="icons:find-in-page"></iron-icon>
On 2016/02/23 21:27:04, dpapad wrote:
> On 2016/02/23 at 02:45:12, Dan Beam wrote:
> > nit: why not
> > 
> >   <iron-icon icon="icons:find-in-page" src="[[engine.iconURL]]">
> >   </iron-icon>
> > 
> > in the iron-icon doc it says [src] wins over [icon] if it's present
> 
> Tried it and it does not work (only the default icon is displayed for all
> entries). Even if that part worked though, I don't think that the
> icon="someIcon" would take over src=<empty string>, an empty icon would be
> displayed for such cases.

the code should support this:
https://github.com/PolymerElements/iron-icon/blob/master/iron-icon.html#L164

but there's a bug from leaving |this.img_| around:
https://github.com/PolymerElements/iron-icon/issues/61

might get fixed soon:
https://github.com/PolymerElements/iron-icon/pull/48

[dpapad, 2016-02-24 01:33:23]

As discussed, because of
https://codereview.chromium.org/1709413003/diff/100001/chrome/browser/resourc...,
the "Add search engine" button is not styled correctly anymore (and I am not
addressing in this CL).

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/1713283002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:26:
<iron-icon icon="icons:find-in-page"></iron-icon>
On 2016/02/23 at 23:59:13, Dan Beam wrote:
> On 2016/02/23 21:27:04, dpapad wrote:
> > On 2016/02/23 at 02:45:12, Dan Beam wrote:
> > > nit: why not
> > > 
> > >   <iron-icon icon="icons:find-in-page" src="[[engine.iconURL]]">
> > >   </iron-icon>
> > > 
> > > in the iron-icon doc it says [src] wins over [icon] if it's present
> > 
> > Tried it and it does not work (only the default icon is displayed for all
> > entries). Even if that part worked though, I don't think that the
> > icon="someIcon" would take over src=<empty string>, an empty icon would be
> > displayed for such cases.
> 
> the code should support this:
> https://github.com/PolymerElements/iron-icon/blob/master/iron-icon.html#L164
> 
> but there's a bug from leaving |this.img_| around:
> https://github.com/PolymerElements/iron-icon/issues/61
> 
> might get fixed soon:
> https://github.com/PolymerElements/iron-icon/pull/48

Thanks for the info, I subscribed to the bug.

[dpapad, 2016-02-24 01:33:29]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-02-24 01:33:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1713283002/#ps140001
(title: "remove paper-button dependency")

[commit-bot: I haz the power, 2016-02-24 01:39:55]

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

[commit-bot: I haz the power, 2016-02-24 03:20:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-24 03:22:08]

Description was changed from

==========
MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles in search engines list.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.
 - Move vertical dots icon closer to the edge.

BUG=479359
==========

to

==========
MD Settings: Manage search engines, polish to match latest mocks.

 - Display default icon when no icon exists for a search engine.
 - Style default search engine with bold text.
 - Drop column titles in search engines list.
 - Remove option to collapse "Other search engines".
 - Move "Add search engine" button.
 - Move vertical dots icon closer to the edge.

BUG=479359

Committed: https://crrev.com/358f1077057fc264978b7bb02d3e35f39421ffab
Cr-Commit-Position: refs/heads/master@{#377204}
==========

[commit-bot: I haz the power, 2016-02-24 03:22:09]

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