MD Settings: Search Engines: Move dropdown menu to search_engines_list

MD Settings: Search Engines: Move dropdown menu to search_engines_list

This moves the dropdown out of the individual list items and
unifies the menu handling code in search_engine_list.

It also updates the CSS to use the shared styling and fixes keyboard focus for 'Add search engine'.

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

[stevenjb, 2016-09-15 22:18:16]

Description was changed from

==========
MD Settings: Search Engines: Move dropdown menu to search_engines_list

This moves the dropdown out of the individual list items and
unifies the menu handling code in search_engine_list.

It also updates the CSS to use the shared styling.

BUG=639718
==========

to

==========
MD Settings: Search Engines: Move dropdown menu to search_engines_list

This moves the dropdown out of the individual list items and
unifies the menu handling code in search_engine_list.

It also updates the CSS to use the shared styling.

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

[stevenjb, 2016-09-15 22:54:36]

Description was changed from

==========
MD Settings: Search Engines: Move dropdown menu to search_engines_list

This moves the dropdown out of the individual list items and
unifies the menu handling code in search_engine_list.

It also updates the CSS to use the shared styling.

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

to

==========
MD Settings: Search Engines: Move dropdown menu to search_engines_list

This moves the dropdown out of the individual list items and
unifies the menu handling code in search_engine_list.

It also updates the CSS to use the shared styling and fixes keyboard focus for
'Add search engine'.

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

[stevenjb, 2016-09-15 22:54:48]

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

[dpapad, 2016-09-15 23:14:13]

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

https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:50:
<iron-dropdown horizontal-align="right" vertical-align="auto">
Can we encapsulate the menu inside its own element, instead of dumping it inside
search-engine-list? Almost all the logic in search_engine_list.js now is related
to the menu, which feels a bit of a violation. For example, the list now keeps
track of a search engine member variable, that is feeding the menu, and is
otherwise useless to the list.

[dpapad, 2016-09-15 23:18:20]

On 2016/09/15 at 23:14:13, dpapad wrote:
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
> File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):
> 
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:50:
<iron-dropdown horizontal-align="right" vertical-align="auto">
> Can we encapsulate the menu inside its own element, instead of dumping it
inside search-engine-list? Almost all the logic in search_engine_list.js now is
related to the menu, which feels a bit of a violation. For example, the list now
keeps track of a search engine member variable, that is feeding the menu, and is
otherwise useless to the list.

Another way this feels like a violation: Unit testing the menu requires creating
a list. Before, I would create a search-engine-entry and test the menu based on
that, which does not seem like a violation, since they both refer to the same
"search engine" model.

[stevenjb, 2016-09-15 23:35:55]

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

https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:50:
<iron-dropdown horizontal-align="right" vertical-align="auto">
On 2016/09/15 23:14:12, dpapad wrote:
> Can we encapsulate the menu inside its own element, instead of dumping it
inside
> search-engine-list? Almost all the logic in search_engine_list.js now is
related
> to the menu, which feels a bit of a violation. For example, the list now keeps
> track of a search engine member variable, that is feeding the menu, and is
> otherwise useless to the list.

We certainly can. Adding new elements is a bit more extra work than adding a C++
class/file, so we often don't do that, but in principal I agree that it is
better encapsulation.

Let me know if you think that is important to do now. I'm juggling about 10
balls right now so this may not return to the top of my queue for a few weeks...
(I'm gardening next week then out for a couple).

[dpapad, 2016-09-15 23:45:46]

On 2016/09/15 at 23:35:55, stevenjb wrote:
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
> File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):
> 
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:50:
<iron-dropdown horizontal-align="right" vertical-align="auto">
> On 2016/09/15 23:14:12, dpapad wrote:
> > Can we encapsulate the menu inside its own element, instead of dumping it
inside
> > search-engine-list? Almost all the logic in search_engine_list.js now is
related
> > to the menu, which feels a bit of a violation. For example, the list now
keeps
> > track of a search engine member variable, that is feeding the menu, and is
> > otherwise useless to the list.
> 
> We certainly can. Adding new elements is a bit more extra work than adding a
C++ class/file, so we often don't do that, but in principal I agree that it is
better encapsulation.
> 
> Let me know if you think that is important to do now. I'm juggling about 10
balls right now so this may not return to the top of my queue for a few weeks...
(I'm gardening next week then out for a couple).

Would it help if I take over this for the search_engines_page? Suggesting this
because:
 1) I don't think anyone will find time to improve the encapsulation later
 2) Suspecting that you'll need to update search_engines_page_test.js in a
non-trivial way, given these changes (does not look like tryjobs have run yet)
and that will require more time than what you already devoted.

[stevenjb, 2016-09-16 03:22:24]

On 2016/09/15 23:45:46, dpapad wrote:
> On 2016/09/15 at 23:35:55, stevenjb wrote:
> >
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
> > File
> chrome/browser/resources/settings/search_engines_page/search_engines_list.html
> (right):
> > 
> >
>
https://codereview.chromium.org/2331423008/diff/20001/chrome/browser/resource...
> >
>
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:50:
> <iron-dropdown horizontal-align="right" vertical-align="auto">
> > On 2016/09/15 23:14:12, dpapad wrote:
> > > Can we encapsulate the menu inside its own element, instead of dumping it
> inside
> > > search-engine-list? Almost all the logic in search_engine_list.js now is
> related
> > > to the menu, which feels a bit of a violation. For example, the list now
> keeps
> > > track of a search engine member variable, that is feeding the menu, and is
> > > otherwise useless to the list.
> > 
> > We certainly can. Adding new elements is a bit more extra work than adding a
> C++ class/file, so we often don't do that, but in principal I agree that it is
> better encapsulation.
> > 
> > Let me know if you think that is important to do now. I'm juggling about 10
> balls right now so this may not return to the top of my queue for a few
weeks...
> (I'm gardening next week then out for a couple).
> 
> Would it help if I take over this for the search_engines_page? Suggesting this
> because:
>  1) I don't think anyone will find time to improve the encapsulation later
>  2) Suspecting that you'll need to update search_engines_page_test.js in a
> non-trivial way, given these changes (does not look like tryjobs have run yet)
> and that will require more time than what you already devoted.

If you can take this on that would be great. We can also discuss this in general
at the standup on Monday (see my comment in the issue).

[dpapad, 2016-10-18 00:49:19]

FYI, you can probably delete/close this CL now.

[dpapad, 2016-11-17 18:44:13]

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