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

Syntax lookup: Fix routing #445

Merged

Conversation

kevanstannard
Copy link
Contributor

Hi @ryyppy

As discussed, this PR is a suggestion for fixing the Syntax Lookup routing.

Notes

[1] I've tried to stay as close to the current implementation as possible and minimise changes, with one exception ...

[2] MAIN CHANGE: In the current code, when the search changes we have an effect to update the URL. In this PR I've reversed that, so when the URL changes it updates the search. I've tried to comment the flow of the code, see what you think.

[3] For route changes I'm using Next.Router.push so we get some history. The replace behaviour has been removed (no longer needed due to point [2] above).

[4] I've removed the GithubSlugger.slug(item.id) code. If we stick to the current id pattern using hyphenated lowercase names then I assume it's not needed? But let me know what you think.

[5] I've added a scrollToTop() when the router triggers a Detail view. The only time this might feel a bit weird is when you have scrolled down the page a bit and manually type in the full name of an item.

[6] BEHAVIOUR CHANGE: One behaviour change compared to the current implementation. If you are on a ShowDetails view and then change the search box, the URL anchor is not cleared.

What do you think of this direction? Do you think that continuing with the current approach of syncing the route to the search (rather than syncing the search to the route) might be worth exploring more?

Testing

  • Navigating to /syntax-lookup shows the ShowAll view.
  • Full page reload of an anchored page such as /syntax-lookup#module-decorator shows the ShowDetails view.
  • Clicking a tag shows the ShowDetails view and sets the URL anchor
  • Clearing the search box via the "x" shows the ShowAll view and clears the URL anchor
  • Clearing the search box using the keyboard shows the ShowAll view and clears the URL anchor
  • Entering text in the search box not matching an item shows the ShowFiltered view, noting that the existing URL anchor remains unchanged.
  • Entering text in the search box exactly matching an item name shows the ShowDetails view.
  • ShowDetails view consistently scrolls to the top.
  • URL history of ShowDetails and ShowAll views are retained.

@ryyppy
Copy link
Member

ryyppy commented Sep 10, 2021

Okay this feels way nicer and fixes quite a few issues. Code changes make sense to me as well. What else is there to explore?

@kevanstannard
Copy link
Contributor Author

Thanks @ryyppy, should be fine to merge as-is. I've also just added links to the raw pages.

@ryyppy
Copy link
Member

ryyppy commented Sep 10, 2021

This is great!

@ryyppy ryyppy merged commit e4a0b35 into rescript-association:master Sep 10, 2021
@kevanstannard kevanstannard deleted the syntax-lookup-fix-routing branch September 10, 2021 11:16
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 this pull request may close these issues.

None yet

2 participants