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

Opening URLs "/notes/new-note" and "/notes/new-notebook" doesn't take us to the proper screens #98

Closed
zecakeh opened this issue Jan 11, 2021 · 5 comments · Fixed by #108

Comments

@zecakeh
Copy link
Contributor

zecakeh commented Jan 11, 2021

It loads the /notes path with { colUid: "new-note" } or { colUid: "new-notebook" }.

Solutions I see:

  • Change the path of those two screens because they collide with /notes/:colUid.
  • Check colUid value in NoteListScreen and redirect to the proper screens if needed.

Maybe this could be taken upstream to React Navigation.

@tasn
Copy link
Member

tasn commented Jan 11, 2021

I think you just need to reorder the screens and then it's fine. So have the new-note and new-notebook before the others. Does that fix it?

@zecakeh
Copy link
Contributor Author

zecakeh commented Jan 12, 2021

It does work in the linking configuration, which is strange to me because it's defined as an object and object properties are not ordered so they shouldn't rely on that…

@tasn
Copy link
Member

tasn commented Jan 12, 2021

That's not good then, it's just a hack if it actually works... It's quite the limitation though if this is not supported... Does react-navigation have anything official regarding this? Can we maybe pass a list instead of a dict?
I guess the better solution though would just be to change the paths as you suggested. Maybe: /notes/note/new and /notes/notebook/new? What's consistent with the rest of them?

@zecakeh
Copy link
Contributor Author

zecakeh commented Jan 12, 2021

Does react-navigation have anything official regarding this? Can we maybe pass a list instead of a dict?

I found this in their v6 roadmap:

  • Drop legacy linking config, validate linking config against navigation state against state changes in development mode and warn about misconfiguration
  • Perform validation of the linking config to identify errors such conflicting screens for patterns on initial render rather than on each invocation of getStateFromPath

So I'm guessing path conflicts are not supported, nor do they plan to support them

Maybe: /notes/note/new and /notes/notebook/new?

This could trigger the NoteEdit screen: /notes/:colUid/:itemUid. But actually so do the collection edit, changelog and members screens.

There is however something I just though of: if we implement the split-view screen, there's gonna be a "master" screen that's gonna receive every route under notes and decide which screen to show on the right so it'll be in charge of handling conflicts anyway. If the screen is not split, we can still use it to decide which screen to show.

@tasn
Copy link
Member

tasn commented Jan 12, 2021

I'm not sure I follow your last statement. Anyhow, cool, let's fix it then so there are no conflicts. :(
Please try to keep thing hierarchical though, so for eaxmple:
/notes/:colUid/:itemUid -> notebook/:colUid/note/:itemUid or something like this.

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

Successfully merging a pull request may close this issue.

2 participants