Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core-dropdown-menu is ugly magugly #1146

Closed
richieb opened this issue Jan 30, 2015 · 11 comments
Closed

core-dropdown-menu is ugly magugly #1146

richieb opened this issue Jan 30, 2015 · 11 comments

Comments

@richieb
Copy link

richieb commented Jan 30, 2015

Come on, Gentlemen. The "inspired" version of core-dropdown-menu seen on the MaterialUI site (see link below) is much sexier and more seductive than the original Polymer core-dropdown-menu:

MaterialUI "inspired" dropdown menu:
http://callemall.github.io/material-ui/#/components/dropdown-menu

Polymer original core-dropdown-menu
https://www.polymer-project.org/components/core-dropdown-menu/demo.html

Surely you can make core-dropdown-menu look and work (drop-down animation needed and mouse-over color for items needed) as well as its sexy siblings, yes? That should be a quick touch up, so the sooner the better :). Polymer shall never be out-sexied, not by anyone, not by Kim K, and definitely not by some overnight MaterialUI youngster.

@richieb
Copy link
Author

richieb commented Jan 30, 2015

In addition, perhaps more importantly, it would be helpful to add an "id" property or even "data-id" on the core-items that are part of the core-dropdown-menu. This way we can programmatically refer to each core-dropdown-menu item not by its label (which can be a long string), but by its data-id (or id).

For example, we should be able to pass an array of objects to core-dropdown-menu:

var listItems = [{label:"First Label", id:"firstItem"}, {label:"Second Label, A Long-Winded One", id:"secondItem"}]

@arthurevans
Copy link

Core-dropdown-menu is a basic, no-frills menu, not a material design version. That would be paper-dropdown-menu:

Demo: https://www.polymer-project.org/components/paper-dropdown-menu/demo.html
Docs: https://www.polymer-project.org/docs/elements/paper-elements.html#paper-dropdown-menu

(The current division of components is a little confusing, because most of the core- elements are unthemed, but a few, like core-header-panel, have a material-design look. This is a known issue, and we'll try to do better about branding the components.)

paper-dropdown-menu doesn't color the selected item, but you can do that easily enough:

paper-dropdown-menu core-item.core-selected {
  color: red;
}

You can't pass an array of items to *-dropdown-menu, but you can use data binding inside a Polymer element or autobinding template to do the same thing:

<template repeat="{{listItems}}">
<paper-dropdown-menu label="Your favorite pastry">
    <paper-dropdown class="dropdown">
        <core-menu class="menu">
            <paper-item id="{{id}}">{{label}}</paper-item>
        </core-menu>
    </paper-dropdown>
</paper-dropdown-menu>
</template>

See: http://jsbin.com/tumaxi/1/edit?html,console,output

Needing to put in all that boilerplate feels kind of ugly, but it does give you a lot of flexibility, IMO.

Lastly, the assumption that the component authors are gentlemen is unwarranted. :D

@richieb
Copy link
Author

richieb commented Jan 30, 2015

@arthurevans, first, thanks for calling me out on my "gentlemen" reference. I apologize to all the wonderful ladies their on your team. I have, in fact, seen some female names (well, at least one :P ), so I deserved being reprimanded for forgetting to give them credit. I instinctively (and incorrectly for sure) used "gentlemen."

Second, thanks for the thorough reply.

Alright, with those important bits out of the way, now for the less important stuff:

  1. I am using paper-dropdown-menu and I am getting a weird flicker (an ugly white screen just before the menu expands) and even the expanding works like the core-menu drop down. I will investigate to see why this is happening, since I don't see it on the demo.
  2. I added my own mouseover css some time ago, but I still think the default component should come with the mouse over effect.
  3. I happen to implement the data binding just as you have, since that is pretty much the only way I know how to do it currently. So I am glad to know I am doing it correctly.

@arthurevans
Copy link

Cool.

For the flashing, it sounds like you might be seeing googlearchive/paper-dropdown-menu#45 ... Can you verify what version of the component you have? I think this was fixed in 0.5.4.

Not sure about the mouseover effect. I know the elements team has been working on reconciling elements with the spec, but I'm don't see that in the sepc. @morethanreal is the menu expert.

@richieb
Copy link
Author

richieb commented Jan 30, 2015

For the flashing, it sounds like you might be seeing googlearchive/paper-dropdown-menu#45 ... Can you verify what version of the component you have? I think this was fixed in 0.5.4.

Yep, that is exactly the problem I have. I was using versions 0.5.2. So I just updated to 0.5.4, but the problem still exists.

@richieb
Copy link
Author

richieb commented Jan 30, 2015

So, even after updating to web-animations 1.0.5 and core-animation 0.5.4, I still see the flickering problem. I just posted a comment to the issue you referenced. I am hopeful someone will follow up, especially since the issue has been marked closed?

On a tangentially related note, I imagine there is a more elegant way than this to programmatically select a paper-dropdown-menu item:

 var dropdownMenu = this.shadowRoot.querySelector("paper-dropdown-menu"),
                            selectedItem = dropdownMenu.querySelector("#" + this.currentItem.id),
                            obj = {
                                detail: {
                                    isSelected: true,
                                    item: {label: this.currentItem.label}
                                }
                            };

                    dropdownMenu.selectAction(obj);
                    selectedItem.classList.add("core-selected");

@arthurevans
Copy link

Interesting. I'm not seeing the flicker in the demo or my jsbin, so it may be specific to your use case. If you could post a jsbin or something over on that bug, it might help identify whether you're seeing the same thing or a new issue.

The core-menu extends core-selector, which provides the .selected property. There are two ways you can use this:

  1. Set valueattr to the name of an attribute you want to use for selection, such as id.
     <core-menu id="menu" valueattr="id"  ... >

To select an item:

    this.$.menu.selected = 'firstItem';
  1. If you'd rather select items by index, set valueattr to an attribute that you're NOT setting on your
    items. (It defaults to name, so if your items don't have a name attribute, you're all set).
To select an item:

       this.$.menu.selected = 1;

This is really not covered well in the core-selector docs. I filed a bug about this:
googlearchive/core-selector#31

@richieb
Copy link
Author

richieb commented Jan 30, 2015

Thanks a lot, @arthurevans. That (the valueattr syntax) works. I didn't try the other one.

The problem with the flickering on the Paper dropdown menu still exists. All the examples on the Polymer site and elsewhere, that I have seen, still have the problem. Note that the problem occurs after the first click; that is, when you click to expand the menu the first time, all is well, but any clicks to expand the menu thereafter results in the flickering problem.

I haven't seen an example anywhere that doesn't have the problem. Could you get your colleague who fixed the issue to confirm that it has indeed been fixed? Or can someone post a working example on jsbin?

@diegohaz
Copy link

diegohaz commented Feb 1, 2015

I'm still seeing the flickering on Chrome 40.

Plus, even considering paper-dropdown, I agree that other material design versions of dropdown-menu are better look than the polymer one.

@amiiit
Copy link

amiiit commented Feb 4, 2015

On Chrome 42 (Version 42.0.2294.0 canary (64-bit)) the flickering problem seems to be resolved.

@arthurevans
Copy link

@richieb So on my end, I haven't seen an example on 0.5.4 that DOES show the original problem. Rather than duplicate comments over here, I'm going to continue this discussion over on the original bug thread (googlearchive/paper-dropdown-menu#45) and close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants