Closed notwaldorf closed 8 years ago
Left 1 comment, will continue reviewing tests tomorrow. Kind of scary that the new element is a copy-paste of the old one for the most part though :x
Yeah, sorry, forgot to add that to the description (added now). I talked to @cdata and we decided that splitting the code into a separate behaviour wasn't good (since behaviours inspire "something you want to reuse a lot"), and this isn't it. When the inheritance branch lands, this can be all cleaned up, since paper-dropdown-menu-light
is just an extension + a slightly different template, but in the meantime, i agree, this isn't great :(
So will this be renamed to and replace paper-dropdown-menu
?
@miztroh Only in a 2.0 release. Removing paper-input
is a breaking change and needs a major version bump, so for now we're shipping it as a separate element. In the 2.0 release, paper-dropdown-menu-light
will probably become paper-dropdown-menu
, along with other potential breaking change improvements.
LGTM :+1:
RTL is ok too now:
Good catch!
Put
paper-dropdown
on the treadmill:paper-input
with a styled div. I had originally made this styled div a separate element (because it's got a bunch of styles), but since we had to bind down all the value/label/focused/disabled etc related properties this was actually a bit expensive.paper-dropdown-menu
in a different PRCode details to make review easier:
paper-dropdown-menu
. The only thing that is new is the<div class="container
and its styles,hasContent
, and the 2 private methods at the end100%
copied frompaper-dropdown-menu
I think this new element and the old one should behave in almost the same way (maybe minor differences re: styling, as we're using different mixins/custom properties). I've duplicated the demo and the tests to demonstrate this.
Results: 3.5x gain (the numbers are for 1k elements) 🎉🎉🎉
Sanity check it looks the same (left:
paper-dropdown-menu
, rightpaper-dropdown-menu-light
. The lavender is the entire dropdown:host
):/cc @cdata