Improve syntax lookup history behaviour #385
Conversation
Minnozz
changed the title
Minnozz
changed the title
node_modules/.bin was missing See npm/cli#2147 (comment)
package-lock.json was updated to v2 in b1ecde6
Use Next.Router.push instead of replace
|
I was hesitant to add a dependency to this project, but I think this is the cleanest way to write this logic. |
|
What makes the logic cleaner with ReactUpdate? It might look cleaner on first glance, but for me it's currently not entirely clear what benefits I get over the approach we had before? |
| /* | ||
| This effect syncs the url anchor with the currently shown final match | ||
| within the fuzzy finder. If there is a new match that doesn't align with | ||
| the current anchor, the url will be rewritten with the correct anchor. | ||
|
|
||
| In case a selection got removed, it will also remove the anchor from the url. | ||
| We don't replace on every state change, because replacing the url is expensive | ||
| */ | ||
| React.useEffect1(() => { | ||
| switch (state, getAnchor(router.asPath)) { | ||
| | (ShowDetails(item), Some(anchor)) => | ||
| let slug = GithubSlugger.slug(item.id) | ||
|
|
||
| if slug !== anchor { | ||
| router->Next.Router.replace("syntax-lookup#" ++ anchor) | ||
| } else { | ||
| () | ||
| } | ||
| | (ShowDetails(item), None) => | ||
| router->Next.Router.replace("syntax-lookup#" ++ GithubSlugger.slug(item.id)) | ||
| | (_, Some(_)) => router->Next.Router.replace("syntax-lookup") | ||
| | _ => () | ||
| } | ||
| None | ||
| }, [state]) |
There was a problem hiding this comment.
@ryyppy For me, the improvement is being able to connect a specific action (like clearing the search box) to this effect (updating the URL). With ReactUpdate, you can queue the effect at the same location where you handle the action, whereas in the old version you have to compare the current URL and new state to reverse engineer what the most recent change was.
|
See inline comment: #385 (comment) Of couse, what is clearer is subjective, and if you want I can try to achieve the same behaviour without this library. |
|
@Minnozz Now that I had a closer look at rescript-react-update, I think your solution is valid, but for now I wanted to keep the dependency footprint low. #445 fixes the router problem (including a few other UX issues), so this should be fixed now. Sorry for my late response times; I try to have a closer eye on open PRs now, in case you see potential improvements in the code. |
(I based this PR on #387 because my development setup was not usable otherwise)
Next.Router.pushinstead ofreplaceto allow the back button to workFixes #383