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

Bug: Do not automatically pushState on internal anchor links #204

Open
davesmith00000 opened this issue May 27, 2023 · 3 comments
Open

Bug: Do not automatically pushState on internal anchor links #204

davesmith00000 opened this issue May 27, 2023 · 3 comments
Labels
bug Something isn't working workaround

Comments

@davesmith00000
Copy link
Member

Frontend routing was introduced in Tyrian 0.7.0, and this was tested for navigation by button press and by anchor link... but not at the same time! 🤦

With the best of intentions, when an anchor link is clicked and the href is determined to be internal, we do a pushState to update the address bar. Makes sense, you've clicked a link and you're going to want to see that link reflected in the address bar.

The problem is that this results in inconsistent behaviour with buttons (that are being used for navigation) where the push state must be done by the user via Nav.pushUrl.

The work around if you have button AND anchor based navigation is very simple, you need two Msg's that both mean 'change page':

  1. The first is for things like buttons, where no-one would expect the address bar to be automatically updated. When this message hits the update function, you also return Nav.pushUrl Cmd if you want the address bar to change.
  2. The second is for anchor (i.e. a(href := "/page")("my link") ) links, which you do NOT use with Nav.pushUrl, just return Cmd.None (or whatever else you want to do) because the address bar has been taken care of.
@davesmith00000
Copy link
Member Author

This is the offending code:

if locationToRoute.isInternal then
// Updates the address bar
window.history.pushState(new js.Object, "", loc)

The fix (if a fix is needed?) is just to remove those three lines.

@davesmith00000
Copy link
Member Author

Hmmm. I've tried this, and it appears to be worse in terms of history management. I must be missing something... 🤔

@TonioGela
Copy link
Contributor

My 2 cents that solved my routing issue:

trait Msg
case class NavigateToInternal(page: Page)  extends Msg
case class NavigateToExternal(url: String) extends Msg
case class InternalRedirect(page: Page)    extends Msg


override def update(state: State.Model): State.Msg => (State.Model, Cmd[IO, State.Msg]) =
  case Page.NavigateToInternal(page) =>
    (if state.page.url === page.url then state else state.copy(page = page), Cmd.None)
  case Page.NavigateToExternal(url)  => (state, Nav.loadUrl(url))
  case Page.InternalRedirect(page)   =>
    (if state.page.url === page.url then state else state.copy(page = page), Nav.pushUrl(page.url))

In short:

  • I added a message for internal routing
  • I check that the URL of the page is the same I'm trying to navigate to to avoid resetting the page state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working workaround
Projects
None yet
Development

No branches or pull requests

2 participants