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

feat: persistence of searchParams for leaderboard #151

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gijs-ve
Copy link
Collaborator

@gijs-ve gijs-ve commented Oct 5, 2024

Added queryParams to leaderboard homepage (#132)

How to test:

  1. Go to homepage
  2. Change filters such as 1v1 supremacy, 50 rows per page, search input or page change
  3. Confirm that the queryParams are changing in the url
  4. Refresh page
  5. Confirm that the initialState is correct and behaving as expected based on the queryParams in the URL

Copy link

vercel bot commented Oct 5, 2024

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

Name Status Preview Comments Updated (UTC)
aom-gg ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2024 0:34am

Copy link
Owner

@erin-fitzpatric erin-fitzpatric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the query params persist when you click into the profile. I'm not entirely sure if they should. We could have different pages within the profile potentially.

Do you think the "type" query param should have value of "1v1", "team", etc. so that the url makes more sense to the user?

Functionality seems great from what I tested, thanks for including the testing steps.

export const useQueryParams = <T extends Record<string, string>>(params: T): QueryParams<T> => {
const router = useRouter();

return useMemo(() => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to using useMemo. As I understand it, this is caching the "page" so that it isn't recalculated when navigating back/forward I assume? Let me know if there was another purpose that I missed. Thanks for adding this.

@gijs-ve
Copy link
Collaborator Author

gijs-ve commented Oct 7, 2024

I noticed that the query params persist when you click into the profile. I'm not entirely sure if they should. We could have different pages within the profile potentially.

Do you think the "type" query param should have value of "1v1", "team", etc. so that the url makes more sense to the user?

Functionality seems great from what I tested, thanks for including the testing steps.

No the persistence for the profile page makes no sense and could be fixed. I was aware of this though, the reason I did initialise the pull request is that I don't consider this something breaking. The parameter is not adjusting anything for the profile page. So yes, definitely make a new issue and I or someone else can fix this at a later stage IMO.

For the type query, it is possible to adjust the params, but for those types that are long and contain spaces, it can be a little bit annoying to maintain since we need to hashmap "Team Deathmatch" to "Team_Deatchmatch". The numbers make even less sense though. But yeah not sure if this is something for the current PR or something that we should change later. Let me know what your preferences are.

@erin-fitzpatric
Copy link
Owner

erin-fitzpatric commented Oct 7, 2024 via email

@erin-fitzpatric
Copy link
Owner

@gijs-ve - hey just was wondering if you were planning on finishing up this PR at some point.

@gijs-ve
Copy link
Collaborator Author

gijs-ve commented Oct 28, 2024

@gijs-ve - hey just was wondering if you were planning on finishing up this PR at some point.

Heya definitely will finish this when I find the time!

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