Add a <cr-dropdown-menu> custom element to cr_elements.

Add a <cr-dropdown-menu> custom element to cr_elements.

BUG=

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

[Jeremy Klein, 2015-02-11 23:44:52]

jlklein@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[Jeremy Klein, 2015-02-11 23:44:58]

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:43: label:
'Select an item',
Anyone have thoughts on i18n here?

[stevenjb, 2015-02-12 18:44:52]

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

[stevenjb, 2015-02-12 18:44:52]

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:43: label:
'Select an item',
On 2015/02/11 23:44:58, Jeremy Klein wrote:
> Anyone have thoughts on i18n here?

The default label should probably be something that is clearly not intentional,
e.g. '<Dropdown Menu Label>'. That said, I think that eventually this element
(and other cr- elements) should take care of localization when 'label' is set
(and when the element is created), which suggests that perhaps we need a
separate 'translatedLabel' property also?

[stevenjb, 2015-02-12 18:47:34]

https://codereview.chromium.org/917893004/diff/1/chrome/browser/resources/md_...
File chrome/browser/resources/md_settings/md_settings.html (right):

https://codereview.chromium.org/917893004/diff/1/chrome/browser/resources/md_...
chrome/browser/resources/md_settings/md_settings.html:25:
<paper-item>Chrome</paper-item>
Will we need cr-item to handle localization, or should that be handled in
cr-dropdown-menu?

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:7: *
'cr-dropdown-menu' is a convenient wrapper around paper-dropdown-menu. It
We should probably standardize these comments. I would suggest something like
'is a chrome specific wrapper around'...

[Jeremy Klein, 2015-02-12 20:32:39]

https://codereview.chromium.org/917893004/diff/1/chrome/browser/resources/md_...
File chrome/browser/resources/md_settings/md_settings.html (right):

https://codereview.chromium.org/917893004/diff/1/chrome/browser/resources/md_...
chrome/browser/resources/md_settings/md_settings.html:25:
<paper-item>Chrome</paper-item>
On 2015/02/12 18:47:34, stevenjb wrote:
> Will we need cr-item to handle localization, or should that be handled in
> cr-dropdown-menu?

I think this is probably a case where the clients would just use cr-i18n, but we
could also make our own cr-item.

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:7: *
'cr-dropdown-menu' is a convenient wrapper around paper-dropdown-menu. It
On 2015/02/12 18:47:34, stevenjb wrote:
> We should probably standardize these comments. I would suggest something like
> 'is a chrome specific wrapper around'...

Done.

https://codereview.chromium.org/917893004/diff/1/ui/webui/resources/cr_elemen...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:43: label:
'Select an item',
On 2015/02/12 18:44:52, stevenjb wrote:
> On 2015/02/11 23:44:58, Jeremy Klein wrote:
> > Anyone have thoughts on i18n here?
> 
> The default label should probably be something that is clearly not
intentional,
> e.g. '<Dropdown Menu Label>'.

Done.

> That said, I think that eventually this element
> (and other cr- elements) should take care of localization when 'label' is set
> (and when the element is created), which suggests that perhaps we need a
> separate 'translatedLabel' property also?

+1, good point. Discussed in the dev meeting.

[michaelpg, 2015-02-19 00:04:45]

I'm seeing cr-dropdown-menu act differently than paper-dropdown-menu.

http://i.imgur.com/BdCVQr5.png

The core-items become visible before the dropdown has expanded.

In a paper-dropdown-menu, the core-items only become visible as the dropdown
expands, as if they're constrained within the dropdown scroller.

https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html
(right):

https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html:8:
<paper-dropdown-menu flex label="{{label}}" opened="{{opened}}">
why is this flex?

[michaelpg, 2015-02-19 22:01:19]

On 2015/02/19 00:04:45, michaelpg wrote:
> I'm seeing cr-dropdown-menu act differently than paper-dropdown-menu.
> 

Looks like the issue is in demo.html (and md_settings.html), not the menu
itself:

https://www.polymer-project.org/docs/elements/paper-elements.html#paper-dropd...

If you add the "menu" class to the core-menu the animation makes more sense.

> http://i.imgur.com/BdCVQr5.png
> 
> The core-items become visible before the dropdown has expanded.
> 
> In a paper-dropdown-menu, the core-items only become visible as the dropdown
> expands, as if they're constrained within the dropdown scroller.
> 
>
https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
> File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html
> (right):
> 
>
https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
> ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html:8:
> <paper-dropdown-menu flex label="{{label}}" opened="{{opened}}">
> why is this flex?

[Jeremy Klein, 2015-02-19 23:44:57]

On 2015/02/19 22:01:19, michaelpg wrote:
> On 2015/02/19 00:04:45, michaelpg wrote:
> > I'm seeing cr-dropdown-menu act differently than paper-dropdown-menu.
> > 
> 
> Looks like the issue is in demo.html (and md_settings.html), not the menu
> itself:
> 
>
https://www.polymer-project.org/docs/elements/paper-elements.html#paper-dropd...
> 
> If you add the "menu" class to the core-menu the animation makes more sense.
> 
> > http://i.imgur.com/BdCVQr5.png
> > 
> > The core-items become visible before the dropdown has expanded.
> > 
> > In a paper-dropdown-menu, the core-items only become visible as the dropdown
> > expands, as if they're constrained within the dropdown scroller.
> > 
> >
>
https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
> > File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
> > ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html:8:
> > <paper-dropdown-menu flex label="{{label}}" opened="{{opened}}">
> > why is this flex?

Weird... I don't see any difference at all with or without the class on my
computer, but I'll add it anyway because the docs say to.

[Jeremy Klein, 2015-02-19 23:45:04]

https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html
(right):

https://codereview.chromium.org/917893004/diff/40001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html:8:
<paper-dropdown-menu flex label="{{label}}" opened="{{opened}}">
On 2015/02/19 00:04:45, michaelpg wrote:
> why is this flex?

This way if a client sets the cr-dropdown-menu's width to 800px or whatever, the
paper-dropdown menu will still stretch to fill it.

[Jeremy Klein, 2015-02-20 22:45:48]

Friendly ping.

[michaelpg, 2015-02-20 22:54:44]

lgtm

https://codereview.chromium.org/917893004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/md_settings/md_settings.html (right):

https://codereview.chromium.org/917893004/diff/80001/chrome/browser/resources...
chrome/browser/resources/md_settings/md_settings.html:23: <cr-dropdown-menu
style="width: 200px">
style should be in md_settings.css

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:48:
assert(menu, 'cr-dropdown-menu must have a menu child with class="menu".');
nit: remove variable

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:49: }
nit: add trailing ,

[Jeremy Klein, 2015-02-20 23:26:59]

New patchsets have been uploaded after l-g-t-m from michaelpg@chromium.org

[Jeremy Klein, 2015-02-20 23:28:13]

https://codereview.chromium.org/917893004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/md_settings/md_settings.html (right):

https://codereview.chromium.org/917893004/diff/80001/chrome/browser/resources...
chrome/browser/resources/md_settings/md_settings.html:23: <cr-dropdown-menu
style="width: 200px">
On 2015/02/20 22:54:44, michaelpg wrote:
> style should be in md_settings.css

I'm adding md_settings.css in https://codereview.chromium.org/939533002/ so if
it's ok with you, I'll just move this in that CL since this seems likely to land
first.

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
File ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js
(right):

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:48:
assert(menu, 'cr-dropdown-menu must have a menu child with class="menu".');
On 2015/02/20 22:54:44, michaelpg wrote:
> nit: remove variable

Done.

https://codereview.chromium.org/917893004/diff/80001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.js:49: }
On 2015/02/20 22:54:44, michaelpg wrote:
> nit: add trailing ,

Done.

[Jeremy Klein, 2015-02-20 23:28:56]

The CQ bit was checked by jlklein@chromium.org

[commit-bot: I haz the power, 2015-02-20 23:30:19]

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

[Dan Beam, 2015-02-20 23:38:30]

chrome/browser/resources/md_settings/md_settings.html lgtm for your CQ to work

[Jeremy Klein, 2015-02-21 00:21:18]

New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org

[Jeremy Klein, 2015-02-21 00:23:21]

The CQ bit was checked by jlklein@chromium.org

[commit-bot: I haz the power, 2015-02-21 00:24:26]

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

[commit-bot: I haz the power, 2015-02-21 01:19:18]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-21 01:20:12]

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