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

Add query params to /super-tokens page for filter + pagination #99

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

thevolcanomanishere
Copy link
Contributor

@kasparkallas This PR includes logic for handling pagination which the previous PR lacked.
I chose not to serialise the filter purely because the URL looks nicer compared to a stringified object in the address bar. There is also some logic in setUrlQueryParam() for handling 1 letter filters.

@vercel
Copy link

vercel bot commented Jun 14, 2022

@thevolcanomanishere is attempting to deploy a commit to the Superfluid Finance Team on Vercel.

A member of the Team first needs to authorize it.

@kasparkallas
Copy link
Collaborator

Appreciate the contribution!

The advantage of serializing into the URL is that it's a general solution we could use for all the tables across Console with hopefully minimal code (and maintenance) per table.

As for the cosmetics of the URL, the serialization process could be extended later to make it comma-separated or something else visually pleasing for the URL, i.e. the serialization doesn't have to be JSON.

@vercel
Copy link

vercel bot commented Jun 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
superfluid-console ✅ Ready (Inspect) Visit Preview Jun 29, 2022 at 9:30AM (UTC)

@thevolcanomanishere
Copy link
Contributor Author

@kasparkallas Implemented serialisation and some small fixes.

@kasparkallas
Copy link
Collaborator

kasparkallas commented Jun 27, 2022

@kasparkallas Implemented serialisation and some small fixes.

Thanks for your effort! I appreciate it and sorry for the slow response.

Looks pretty good but some feedback:

  1. The handling of tab changes doesn't seem right as it's not persisted in the URL: image
  2. Instead of modifying window.history directly, I'd do it through Next.js' router: https://nextjs.org/docs/api-reference/next/router
  3. Instead of explicitly doing setUrlQueryParam in the click handlers, it might be better to handle it in a single useEffect. (Because essentially the act of reflecting the query filters in the URL is a side-effect which is exactly what useEffect is meant for: https://reactjs.org/docs/hooks-effect.html)

The 2. and 3. are not critical and are more related to perfect code structure than feature correctness. I can also tackle these myself at some point.

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.

2 participants