-
Notifications
You must be signed in to change notification settings - Fork 50
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: upgrade react router to v6 #850
feat: upgrade react router to v6 #850
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #850 +/- ##
==========================================
+ Coverage 66.97% 67.17% +0.19%
==========================================
Files 128 128
Lines 3201 3211 +10
Branches 927 927
==========================================
+ Hits 2144 2157 +13
+ Misses 1008 1005 -3
Partials 49 49
☔ View full report in Codecov by Sentry. |
435827b
to
1aeb7c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments. Mostly looks good.
let store; | ||
|
||
const createWrapper = (state) => { | ||
store = mockStore(state); | ||
mockOnSubmit = jest.fn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove this mock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was unnecessary, mockOnSubmit
was used to mock the createCourse
behavior but there were no assertions being made. CreateCoursePage
component was already defining createCourse
as an empty function in its default props, so there was no need to mock it.
But if you still suggest bringing this back there are no objections from my side as it will not affect the router upgrade. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on local working fine, will give it a go on stage as well after merge 🤞🏻
17e84a6
to
3df392a
Compare
9444dbe
to
fb93ee0
Compare
…to Ali-Abbas/react-router-upgrade
Ticket
React Router Upgrade to v6.
Description
This PR upgrades React Router from
v5
tov6
.