search

openedx / frontend-app-discussions

A React-based micro frontend for the Open edX discussion forums.
GNU Affero General Public License v3.0
6 stars 62 forks source link

feat: upgrade react router to v6 #542

Closed Syed-Ali-Abbas-Zaidi closed 9 months ago

Syed-Ali-Abbas-Zaidi commented 1 year ago

Ticket

React Router Upgrade to v6.

Description

This PR upgrades React Router from v5 to v6.

awais-ansari commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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

codecov[bot] commented 10 months ago

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%.

:exclamation: 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 :warning:
src/discussions/post-comments/PostCommentsView.jsx 50.00% 2 Missing :warning:
src/discussions/posts/post/Post.jsx 50.00% 2 Missing :warning:
src/discussions/utils.js 60.00% 2 Missing :warning:
src/discussions/data/hooks.js 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Syed-Ali-Abbas-Zaidi commented 9 months ago

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

Fixed. 🤞

Syed-Ali-Abbas-Zaidi commented 9 months ago

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.