search

erikringsmuth / app-router

Router for Web Components
https://erikringsmuth.github.io/app-router/
MIT License
611 stars 83 forks source link

Multiple instances of app-router #54

Closed abourget closed 9 years ago

abourget commented 9 years ago

I'm using app-router to populate a sidebar navigation as well as the content of an app. It goes like this:

app-router

and inside some other nodes: app-router

I poked at the code, and it seems the second app-router isn't taken into account, because the first one alters the app-router global previousUrl, and the second pass is short-circuited here https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L145

I would really love app-router to be able to handle multiple instances again. Can't we store the previousUrl on the node itself ?

thanks!

abourget commented 9 years ago

I see another problem, it's that when I use this.$.router.go("/somewhere") ... it doesn't notify the other app-router instances.. could we go through the event system all the time ?

abourget commented 9 years ago

I've open a PR for that: https://github.com/erikringsmuth/app-router/pull/55

erikringsmuth commented 9 years ago

With previousUrl, would you check if it was the same in the activate-route-start event and prevent the outer route from loading?

Maybe the app-route should also have an attribute to prevent the route from reloading if it is the same route.

<app-route path="/builder/*" element="builder-el" dontReload></app-route>

It'd need a better name but the idea is there.

router.go() could be modified to use events. Right now it calls stateChange(). https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L121

This could be changed to fire a pushstate event like this. https://github.com/erikringsmuth/pushstate-anchor/blob/master/src/pushstate-anchor.js#L17

It's a little hacky but it works.

erikringsmuth commented 9 years ago

Oh no! I just realized what's wrong here. https://github.com/erikringsmuth/app-router/blob/master/src/app-router.js#L145

I need an additional check at the end.

&& url.hash !== previousUrl.hash
erikringsmuth commented 9 years ago

I changed router.go('/path') so that it fires a popstate event instead of calling stateChange(). This should be picked up by multiple routers and all other functionality should stay the same.

I also added this additional check in stateChange().

url.hash !== previousUrl.hash

These changes are tagged with version 2.3.2 https://github.com/erikringsmuth/app-router/commit/1bd2d50d2b51c0c5f87d70c203aa5a8295929aac

Try this out and see if it covers your use case. If it still needs previousUrlmade public or an attribute option we can make further changes.

erikringsmuth commented 9 years ago

I added a new feature in v2.5.0 to prevent the outer router from reloading. https://erikringsmuth.github.io/app-router/#/api#onurlchange