search

PolymerElements / iron-selector

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

Fixes #94 (`selected` does not update when `attr-for-selected` changes.) #104

Closed MeTaNoV closed 8 years ago

MeTaNoV commented 8 years ago

fixes Issue #94 when attrForSelected change without reflecting the change to selected both in selectable and multi-selectable

bicknellr commented 8 years ago

Thanks for submitting this! I left a few comments for you to look through. Also, could you fill out the tests a bit more? For example, there aren't any tests for when multiple selection is enabled. Otherwise, it looks good.

bicknellr commented 8 years ago

Also, way too many of my sentences are structured like this.

MeTaNoV commented 8 years ago

I'll add a test for multi, oki doki. What about your last remark? :) Like common answer to a PR? then use a template! :)

MeTaNoV commented 8 years ago

Hi Russel ( @bicknellr ), thanks for the follow-up, I took your feedback into account, apart the async() usage which make the test fail if not used. Would be interested to know if such syncing issue could be avoided...

MeTaNoV commented 8 years ago

@bicknellr sorry for the delay... Took into account your last remark. The async usage being the last question to be solved...

bicknellr commented 8 years ago

I think I found out what's going on here after running into the same issue lately. There's a problem with the current implementation of Polymer.Base.async playing poorly with anything using a MutationObserver. Particularly, async uses its own MutationObserver to get microtask timing and flushes it's queue of async callbacks all at once and since async's MutationObserver is pretty much always registered before anything else (when Polymer itself loads) it always runs before any other MutationObservers. So, first register + batching = all async callbacks always run before any MutationObserver callbacks triggered in the same stack. There's a function Polymer.dom.flush(); which force any updates queued by shady DOM to be reflected immediately. I believe it ties into observeNodes (Polymer's shady DOM MutationObserver polyfill-ish thing) and will cause them to flush also.

tl;dr Put Polymer.dom.flush(); between the time the fixtures are created and when the assertion is made and these seem to work. I'll ask around to make sure this is the right idea but wct-ing with all the usual browsers seems to work so I'm pretty confident.

bicknellr commented 8 years ago

Also, when attrForSelected changes and (one of) the selected node(s) doesn't have that key, what happens? I'm worried that it'll add undefined to the set of selected values and then select all nodes without that key. Maybe nodes without the newly chosen key should be filtered out before the values are mapped?

MeTaNoV commented 8 years ago

good to go! :)

bicknellr commented 8 years ago

Left a comment; otherwise, I think this is ready!

MeTaNoV commented 8 years ago

that should be it!

bicknellr commented 8 years ago

My bad, I was under the impression that 0 != null for some reason.. Could you drop the last commit?

MeTaNoV commented 8 years ago

:) done!

bicknellr commented 8 years ago

Awesome, thanks for all the work!