Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Programatic navigation #85

Closed
josephg opened this issue Jun 26, 2016 · 14 comments
Closed

Feature request: Programatic navigation #85

josephg opened this issue Jun 26, 2016 · 14 comments
Milestone

Comments

@josephg
Copy link

josephg commented Jun 26, 2016

I've ended up using this action to navigate between pages:

  effects: {
    navigate: (action, state, send) => {
      send('app:location', {location: action.location});
      window.history.pushState({}, null, action.location);
    }
  }

It'd be nice if there was an officially recommended way to do it. (This or something else.)

@timwis
Copy link
Member

timwis commented Jun 26, 2016

Hey @josephg, we were just talking about this (see #21). It looks like the best way may be to use the native DOM API (window.location.hash = 'foo'). For browser history, I can't member if it's window.location.url or pushState as you have above. But the point is, adjusting those triggers the internal router. And the nice thing about this is that you don't have to memorize a new framework-specific way to change the page/URL. How's that sound?

@yoshuawuyts yoshuawuyts added bug and removed bug labels Jun 26, 2016
@josephg
Copy link
Author

josephg commented Jun 27, 2016

If we can pull it off, perfect. I tried that first hoping it would automatically work. Thanks!

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jun 30, 2016

@josephg Did you end up resolving this - can this be closed?

@josephg
Copy link
Author

josephg commented Jun 30, 2016

Well if thats the intended behaviour, it doesn't work yet:

const choo = require('choo');
const app = choo();

const nav = (url) => {
  console.log('nav to', url);
  window.history.pushState(null, null, url);
}

app.router((route) => [
  route('/', (params, state, send) => choo.view`
    <button onclick=${() => nav('/foo')}>Nav</button>
  `),

  route('/foo', (params, state, send) => choo.view`<h1>Somewhere else</h1>`)
]);

document.body.appendChild(app.start());

... The button changes the URL but the new route isn't rendered.

@timwis
Copy link
Member

timwis commented Jun 30, 2016

window.location.href = '/foo' works for me with your code

@josephg
Copy link
Author

josephg commented Jun 30, 2016

@timwis: That forces an unnecessary full page re-render, which I'm trying to avoid.

@josephg
Copy link
Author

josephg commented Jul 1, 2016

Also @yoshuawuyts: Taking a look at the browser's API, I don't think there's any way for the framework to find out when pushState / replaceState is called.

From MDN:

Note that just calling history.pushState() or history.replaceState() won't trigger a popstate event. The popstate event is only triggered by doing a browser action such as clicking on the back button (or calling history.back() in JavaScript).

So for simple navigation, pushState will need to be wrapped somewhere. It can happen in the app - using something like the code I posted at the top of this issue. Or it can be in choo itself.

@yoshuawuyts
Copy link
Member

@josephg you're right - labelling this a bug; we should fix this

@MattMcFarland
Copy link
Contributor

So what would the appropriate fix be?

If someone changes the window.location, are you saying we need to let choo know somehow? I would prefer that this is done manually, or at least by somehow setting up a subscription to the window.

At anyrate, can someone explain what the problem / intended result should be? I'm not clear on the issue, the original post actually seems like a reasonable approach to me and it doesnt look like a bug. So I must be reading this wrong.

@mantoni
Copy link
Contributor

mantoni commented Aug 2, 2016

@MattMcFarland It's not a bug. It's a feature request to make it possible to change the location with a single API call (e.g. send('app:location', {location: action.location});) without the need to manually fiddle with pushState. Does that answer your question?

@MattMcFarland
Copy link
Contributor

@mantoni Yes thank you!

Is this a feature we are going to add? If so I could work on this providing nobody else is. I"m just looking for something to do :)

@mantoni
Copy link
Contributor

mantoni commented Aug 2, 2016

@MattMcFarland I saw that @yoshuawuyts was looking into it recently and concluded that this would be a breaking change (see his last comment here).

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Aug 2, 2016

[oops mispasted comment, moved it to https://github.com//issues/65#issuecomment-236914727]

@yoshuawuyts
Copy link
Member

Yup I think this should all work now in v4 - closing! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants