search

PolymerElements / paper-dropdown-menu

A Material Design browser select element
https://www.webcomponents.org/element/PolymerElements/paper-dropdown-menu
61 stars 107 forks source link

fix paper-dropdown-light styles #160

Closed notwaldorf closed 8 years ago

notwaldorf commented 8 years ago

Fixes https://github.com/PolymerElements/paper-dropdown-menu/issues/143

Also fixed how we centered the text and the icon: originally I used a min-height on the text, of the same height as the icon, but that's silly if you ever change the font size. Fixed that with some math.

mgiuffrida commented 8 years ago

cool, seems to address the width issue in flexbox from #143: https://jsfiddle.net/30wysv8m/3/

(still differences outside of flexbox -- the paper-dropdown-menu is 6px wider than the paper-dropdown-menu-light)

mgiuffrida commented 8 years ago

the latest changes the dropdown arrow vertical positioning. this fiddle will draw from the fix-light-flex branch if you'd like to see what i'm seeing.

notwaldorf commented 8 years ago

Fixed! I think. https://jsfiddle.net/nah01sgr/

notwaldorf commented 8 years ago

Firefox seems to not want to run tests right now?

mgiuffrida commented 8 years ago

sweet, looks better!

although, the dropdown arrow still isn't vertically aligned (please don't hate me). i could file it as a separate bug if you'd like to let it rest for now.

notwaldorf commented 8 years ago

@mgiuffrida I think that issue was present before too, and I'm not sure it can be fixed easily (the dropdown is absolutely positioned, which makes everything harder).

You can open it as a separate issue -- It doesn't affect you, right? It's just a nice-to-have?

notwaldorf commented 8 years ago

@mgiuffrida Does this look good? Should I send it out for review?

mgiuffrida commented 8 years ago

lgtm!

notwaldorf commented 8 years ago

@e111077 @bicknellr PTAL? I'll do a rebase if everything else looks right

e111077 commented 8 years ago

Looks pretty close, but I'm still getting default width issues. See: image Chrome Linux: Width of the normal one is 225px and the light is 200px. Firefox Linux: 219px and 200px respectively.

I can see this being a separate issue as well. @bicknellr WDYT?

mgiuffrida commented 8 years ago

@e111077 is right. this is much more pronounced when a larger font size is used: https://jsfiddle.net/30wysv8m/7/ (widen the result to see the difference)

bicknellr commented 8 years ago

I'm seeing the same width issue but I'd say it's unrelated to #143. Using em would probably handle it reasonably; I don't think we need to be totally precise in the unset width case given that using both paper-dropdown-menu and paper-dropdown-menu-light on the same page would defeat the purpose of paper-dropdown-menu-light. (But bonus points if it manages to work somehow.)

e111077 commented 8 years ago

Width issue is also borked on master flex or no flex. I say it's unrelated to #143 and flex; suggest opening a different PR & issue if deemed necessary, because @bicknellr brought up a good point about performance.

LGTM to this PR

mgiuffrida commented 8 years ago

agreed, slgtm2

notwaldorf commented 8 years ago

Done did a rebase, merging once the tests come up green