search

RiotGear / rg-router

State based router for Riot apps
MIT License
28 stars 3 forks source link

Save the full state object when using pushState #6

Closed andrefgneves closed 8 years ago

andrefgneves commented 8 years ago

This will correctly restore states with the same name but with different params

gregorypratt commented 8 years ago

Hi, thanks for the PR. I'm not sure I fully understand the purpose of it though. What would the use case for this be? I'm not sure why the same state should be re-used with different params?

andrefgneves commented 8 years ago

My pleasure :)

A simple example would be a paginated list, with a route like:

rg.router.add({ name: 'news', url: 'news/:page' })

where you populate a list based on the page index. We don't want to create a route for each page index. Before this PR the state params (the page in this case) would not be correctly restored, since you were only saving the state name.

You can easily test this by reverting the changes on only the router.js file and trying the demo and pressing the browser's back/forward buttons.

Maybe I got it wrong, but I could not get this use case to work without this change.

gregorypratt commented 8 years ago

Right got it. So instead of state name you're using the uniqueness of the state itself. Could you get latest and see if the other examples still work on the demo page? They don't seem to work for me. Maybe I've done something wrong....

gregorypratt commented 8 years ago

@andrefgneves Ignore my previous comment. The only one not working is the About More demo where it should wait for the resolve property before changing state...could you take a look?

andrefgneves commented 8 years ago

@gregorypratt sure, I'll take a look

gregorypratt commented 8 years ago

I think it's to do with storing the whole state instead of just the name in history...?

Greg

On 24 Nov 2015, at 15:48, André Neves notifications@github.com wrote:

@gregorypratt sure, I'll take a look

— Reply to this email directly or view it on GitHub.

gregorypratt commented 8 years ago

P.s. I've updated the demo to make it easier to comprehend what is going on!

andrefgneves commented 8 years ago

Can you try again with #7 ?

It was failing because that state had a non serializable property (the resolve), so the browser was throwing and error and refusing to push the state. It should be working fine now.