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: upgrade react router to v6 #542

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

Syed-Ali-Abbas-Zaidi
Copy link
Contributor

@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi commented Jun 20, 2023

Ticket

React Router Upgrade to v6.

Description

This PR upgrades React Router from v5 to v6.

@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi self-assigned this Jun 20, 2023
@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi marked this pull request as draft June 20, 2023 04:42
@awais-ansari
Copy link
Contributor

Hi @Syed-Ali-Abbas-Zaidi, Just to confirm. Are you still working on this PR? We are planning the board cleanup. Should I close this PR?

@Syed-Ali-Abbas-Zaidi
Copy link
Contributor Author

Hi @Syed-Ali-Abbas-Zaidi, Just to confirm. Are you still working on this PR? We are planning the board cleanup. Should I close this PR?

Hi, This PR is currently blocked due to this issue in react-router v6. This issue was resolved last week but it will be released in a week or two and after that, we will start working on this PR.

@awais-ansari
Copy link
Contributor

@Syed-Ali-Abbas-Zaidi We are doing some optimization work and decomposition of tightly coupled components to prevent unwanted re-rendering. This PR will be affected by that work. We need to add a little bit of delay for this upgrade to prevent rework. what is your thought on this?
Will you continue working on this branch or create a new one after optimization work?

@awais-ansari
Copy link
Contributor

@Syed-Ali-Abbas-Zaidi You can start working on this PR.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (b36c026) 92.42% compared to head (2186012) 92.35%.

❗ Current head 2186012 differs from pull request most recent head 004360e. Consider uploading reports for the commit 004360e to get more accurate results

Files Patch % Lines
...c/discussions/discussions-home/DiscussionsHome.jsx 81.81% 2 Missing ⚠️
src/discussions/post-comments/PostCommentsView.jsx 50.00% 2 Missing ⚠️
src/discussions/posts/post/Post.jsx 50.00% 2 Missing ⚠️
src/discussions/utils.js 60.00% 2 Missing ⚠️
src/discussions/data/hooks.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   92.42%   92.35%   -0.07%     
==========================================
  Files         169      169              
  Lines        3446     3468      +22     
  Branches      897      900       +3     
==========================================
+ Hits         3185     3203      +18     
- Misses        241      245       +4     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi force-pushed the Ali-Abbas/react-router-upgrade branch 4 times, most recently from 7865d7b to a0f2d5d Compare November 21, 2023 07:52
@Syed-Ali-Abbas-Zaidi Syed-Ali-Abbas-Zaidi marked this pull request as ready for review November 21, 2023 07:53
Copy link
Contributor

@sundasnoreen12 sundasnoreen12 left a comment

Choose a reason for hiding this comment

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

src/discussions/learners/LearnersView.jsx Outdated Show resolved Hide resolved
src/discussions/posts/PostsView.test.jsx Outdated Show resolved Hide resolved
src/discussions/discussions-home/DiscussionSidebar.jsx Outdated Show resolved Hide resolved
src/discussions/posts/post/PostLink.jsx Show resolved Hide resolved
src/discussions/post-comments/PostCommentsView.jsx Outdated Show resolved Hide resolved
src/discussions/discussions-home/DiscussionSidebar.jsx Outdated Show resolved Hide resolved
src/data/constants.js Outdated Show resolved Hide resolved
src/data/constants.js Outdated Show resolved Hide resolved
src/data/constants.js Outdated Show resolved Hide resolved
Copy link
Contributor

@sundasnoreen12 sundasnoreen12 left a comment

Choose a reason for hiding this comment

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

The route is crashing; it should redirect to All Posts for both legacy and new edX.

@Syed-Ali-Abbas-Zaidi
Copy link
Contributor Author

The route is crashing; it should redirect to All Posts for both legacy and new edX.

Fixed. 🤞

Copy link
Contributor

@sundasnoreen12 sundasnoreen12 left a comment

Choose a reason for hiding this comment

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

PR LGTM

Copy link
Contributor

@awais-ansari awais-ansari left a comment

Choose a reason for hiding this comment

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

Some routes are breaking and unexpected behavior. I have listed down all the router bugs and expected behavior in this doc.

@Syed-Ali-Abbas-Zaidi
Copy link
Contributor Author

Some routes are breaking and unexpected behavior. I have listed down all the router bugs and expected behavior in this doc.

I have fixed the issues, and as per our discussion all routes that are not supported by this MFE will redirect to All Posts.

@awais-ansari awais-ansari merged commit b35632d into master Dec 7, 2023
5 checks passed
@awais-ansari awais-ansari deleted the Ali-Abbas/react-router-upgrade branch December 7, 2023 13:10
dcoa pushed a commit to eduNEXT/frontend-app-discussions that referenced this pull request Apr 30, 2024
* feat: upgrade react router to v6

* fix: routing issues

* fix: category route should redirect to all posts

* fix: path error on routes
bra-i-am added a commit to eduNEXT/frontend-app-discussions that referenced this pull request May 2, 2024
* feat: upgrade react router to v6 (openedx#542)

* feat: upgrade react router to v6

* fix: routing issues

* fix: category route should redirect to all posts

* fix: path error on routes

* perf: add support of css-variables to quince

---------

Co-authored-by: Syed Ali Abbas Zaidi <88369802+Syed-Ali-Abbas-Zaidi@users.noreply.github.com>
Co-authored-by: Diana Catalina Olarte <diana.olarte@edunext.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants