PolymerElements / iron-selector

Manages a list of elements that can be selected
32 stars 55 forks source link

toggle only items that really changed selection in multi mode #76

Closed reinert closed 8 years ago

reinert commented 8 years ago

Fixes #75

bicknellr commented 8 years ago

Thanks for sending this PR; awesome tests! PTAL at the comments.

reinert commented 8 years ago

Sorry for the delay. It's been a while and I needed to schedule a time to review it.

Thanks for appreciating the PR. I tried to follow your recommendations, except the first, which I stuck to the pattern already present at the code.

If you feel the bind is really beneficial to the code, I can change it too.

Thanks for your time.

reinert commented 8 years ago

Using MockInteractions on tests broke the patch. Some exception is occurring. Locally in my machine it is working fine. Do you know any reason why is that?

reinert commented 8 years ago

Checking master code, I saw sometimes using CustomEvent, other MockInteractions.

The difference is that MockInteractions is always used wrapping the source element with Polymer like MockInteractions.tap(Polymer.dom(s).children[0]). Is that trick mandatory?

CustomEvent example MockInteractions example

reinert commented 8 years ago

Oh! After merging locally this patch with master I realized this failure is already present on master. So it's not an issue here.

bicknellr commented 8 years ago

Left a comment. Also, #108 should fix the test, so once that's in you'll need to rebase to see it reflected here.

bicknellr commented 8 years ago

108 is in, could you rebase?

reinert commented 8 years ago

Done.

bicknellr commented 8 years ago

LGTM, thanks again!

reinert commented 8 years ago

Thanks for reviewing the PR. You guys are doing an amazing job with the polymer project. My codes were never so beautiful. I'm definitely in love with it.

bicknellr commented 8 years ago

:)