Skip to content

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