Skip to content

[legacy-framework] Added params and query to useRouter and withRouter#677

Merged
flybayer merged 11 commits intoblitz-js:canaryfrom
toshi1127:added-params-and-query-to-use-router-and-with-router
Jun 16, 2020
Merged

[legacy-framework] Added params and query to useRouter and withRouter#677
flybayer merged 11 commits intoblitz-js:canaryfrom
toshi1127:added-params-and-query-to-use-router-and-with-router

Conversation

@toshi1127
Copy link
Collaborator

@toshi1127 toshi1127 commented Jun 13, 2020

This PR closes the issue of Add params and query to useRouter and withRouter. blitz-js/legacy-framework#936

What are the changes and their implications?

Extend useRouter and withRouter with useParams and useRouterQuery hooks.

Current:

const {query} = useRouter()

Change to:

const {params, query} = useRouter()

Checklist

@toshi1127
Copy link
Collaborator Author

Should update the documentation and implement the test code in this PR?

Zeko369
Zeko369 previously approved these changes Jun 13, 2020
Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @toshi1127!!

I've left a few initial comments. I'll do a more in depth review and testing within a day or two.

It's not a big deal, but I think all the renamed imports like WithRouterPropsNext would be better formatted as useNextRouter instead of useRouterNext. Because here in English "Next" is an adjective describing "Router", so "next" should be before "router" :)

@@ -0,0 +1,22 @@
import React from 'react'
import {withRouter as withRouterNext, NextRouter as NextRouterNext} from 'next/router'
Copy link
Member

Choose a reason for hiding this comment

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

You only need 'next' one time in NextRouterNext :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Admittedly, this renaming is unnecessary.
Fixed.

@toshi1127
Copy link
Collaborator Author

Thanks for the review !!
Admittedly, given the grammar, useNextRouter, withNextRouter is appropriate.
These have been fixed.

@flybayer
Copy link
Member

@toshi1127 awesome, looks great!

One last thing. Can you open a PR to update the router object documentation for this? Here's the file on Github

@toshi1127
Copy link
Collaborator Author

@flybayer
Thank you.
Of course I can update the document.
I'll create a PR when I'm done updating it.

@toshi1127
Copy link
Collaborator Author

Created a PR to update the documentation of the router object.

@flybayer
Copy link
Member

Great, thank you! :)

@flybayer
Copy link
Member

@all-contributors add @toshi1127 for docs

@allcontributors
Copy link

@flybayer

I've put up a pull request to add @toshi1127! 🎉

@itsdillon itsdillon changed the title Added params and query to useRouter and withRouter [legacy-framework] Added params and query to useRouter and withRouter Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants