-
Notifications
You must be signed in to change notification settings - Fork 32
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 #1001
Conversation
… into Ali-Abbas/react-router-upgrade
7f74ac0
to
64b95cb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1001 +/- ##
==========================================
- Coverage 85.42% 85.33% -0.09%
==========================================
Files 494 495 +1
Lines 10625 10639 +14
Branches 2219 2230 +11
==========================================
+ Hits 9076 9079 +3
- Misses 1507 1518 +11
Partials 42 42 ☔ View full report in Codecov by Sentry. |
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.
Great job with the PR. Noticed a couple issues QAing the learner credit management section.
@@ -147,7 +147,7 @@ const NewAssignmentModalButton = ({ enterpriseId, course, children }) => { | |||
}); | |||
|
|||
// Navigate to the activity tab | |||
history.push(pathToActivityTab); | |||
navigate(pathToActivityTab); |
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.
Noticed a bug with this implementation where the url is not being changed to reflect the activity tab and is not redirecting correctly. (See video)
The first half of the video is the PR, while the second half is the current stage environment.
https://github.com/openedx/frontend-app-admin-portal/assets/82611798/3e4568f3-5ecd-4501-aa5e-69e11f611194
pathname: generatePath(routeMatch.path, { budgetId, activeTabKey: 'catalog' }), | ||
state: { budgetActivityScrollToKey: 'catalog' }, | ||
}} | ||
to={generatePath(pathname, { budgetId, activeTabKey: 'catalog' })} |
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.
Noticed a bug with this where when the user is on the activity
tab and selects the New course assignment button, the tab no longer changes to the catalog tab. The same behavior can be seen from clicking the View spent activity
and View assigned activity
button from the utilization details button (See Video). For those buttons, when the user is on the catalog tab, the tab is supposed to switch to the activity tab and scroll down to the selected component. (Found in BudgetDetailPageOverviewUtilization
Screen.Recording.2024-01-16.at.10.06.03.AM.mov
pathname: generatePath(routeMatch.path, { budgetId, activeTabKey: 'activity' }), | ||
state: { budgetActivityScrollToKey: type }, | ||
}} | ||
to={generatePath(pathname, { budgetId, activeTabKey: 'activity' })} |
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.
See comment above relating to the tabs not switching.
… into Ali-Abbas/react-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.
LGTM 👍🏽 Will undergo thorough testing in stage before moving over to prod. Thanks for putting all the effort into this.
This reverts commit 4b56103.
Ticket
React Router Upgrade to v6.
Description
This PR upgrades React Router from
v5
tov6
.