Skip to content
This repository was archived by the owner on Mar 13, 2018. It is now read-only.

Initialize the selector's routes asynchronously so that the managed#40

Open
jklein24 wants to merge 1 commit intomasterfrom
asyncselect
Open

Initialize the selector's routes asynchronously so that the managed#40
jklein24 wants to merge 1 commit intomasterfrom
asyncselect

Conversation

@jklein24
Copy link
Copy Markdown
Contributor

@jklein24 jklein24 commented Jun 2, 2015

selector is ready.

This resolves a broken initialization when using more-route-selector with an attr-for-selected property on the managed selector.

@jklein24
Copy link
Copy Markdown
Contributor Author

jklein24 commented Jun 3, 2015

Note: I'm a little concerned about this potentially breaking some default route handling as follows -

Say there's some element which, in its ready callback, checks if the managed selector has a valid selection and, if not, it sets the selected property to the first item. Because this logic was all synchronous before, it would work. This change would conflict with that in strange ways.

Is there another option here? It seems like this is all asking for race conditions...

@morethanreal
Copy link
Copy Markdown

I don't think there's a synchronous way to wait for all your children to become ready. If there's a task that should be performed after the child becomes ready, can it notify the parent to perform some task instead of the parent waiting?

@jklein24
Copy link
Copy Markdown
Contributor Author

jklein24 commented Jun 4, 2015

Hmm.. That could be a little tricky. Our situation is something like

<more-route-selector>
  <iron-pages>
     <a11y-settings-page>
     <internet-settings-page>
     etc.
  </iron-pages>
</more-route-selector>

The selector needs to wait for both the iron-pages and individual page elements to be ready before initializing with _updateRoutes. Maybe I could make that function public and call it in the ready callback of the wrapping element which has all this stuff in its local dom. WDYT?

@morethanreal
Copy link
Copy Markdown

Yeah, that's a possible solution.

It seems like the problem to be solved is to update routes when pages become ready. Does this element handle pages being inserted dynamically? If it does, it seems like you can delay updating routes at initialization if there are no pages and handle the routes when the pages are inserted, e.g. with a MutationObserver.

@nevir
Copy link
Copy Markdown
Contributor

nevir commented Jun 4, 2015

It seems like the problem to be solved is to update routes when pages become ready. Does this element handle pages being inserted dynamically? If it does, it seems like you can delay updating routes at initialization if there are no pages and handle the routes when the pages are inserted, e.g. with a MutationObserver.

Untested; but I tried to code most of it with that in mind. AFAIR, as long as _updateRoutes is called after each mutation, it should be good? :S

@nevir
Copy link
Copy Markdown
Contributor

nevir commented Jun 4, 2015

Another option might be to have some sort of selector-ready event that the observed element fires? (since 99% of the time we're observing something that mixes in IronSelectableBehavior)

@nevir
Copy link
Copy Markdown
Contributor

nevir commented Jun 4, 2015

edit: nevermind. That only supports one level deep, not whether the pages are ready :(

@morethanreal
Copy link
Copy Markdown

We had a discussion about this yesterday and the consensus is there will be a distributedNodesChanged callback or something like that, so you can perform actions that should happen when your distributed children changes. (Using a MutationObserver today won't observe when there is reprojection)

@jklein24
Copy link
Copy Markdown
Contributor Author

jklein24 commented Jun 5, 2015

That sound like what we need for sure... In the mean time, is it worth landing this or #42?

@morethanreal
Copy link
Copy Markdown

Yeah, lgtm

@addyosmani
Copy link
Copy Markdown
Contributor

@jklein24 My vote is to land this one over #42. Let's discuss whether we're supporting more-routing though. We may want to deprecate it given it was never quote official.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants