Migrate user dashboard and Explore pages to SPA #2731
Conversation
|
Hello @anshg1214! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-02-28 13:25:14 UTC |
There was a problem hiding this comment.
Looking better and better! I think we're just about ready to merge after this last pass.
Test suite is currently failing because of the commented out UserEntityChart tests. (see comment).
It wouldn't be wise to comment out the test suite for a feature being refactored and I know this test suite can be tricky, so don't hesitate if you need a hand with it.
As we both discussed in private messages, I think after merging this we'll want to look at our bundling and code-splitting strategy because this index.js file is going to be pretty huge now :)
I don't think we can adapt the existing tests to work with react-router, and we most likely need to rewrite the tests entirely to use react-testing-library instead of Enzyme. In the meantime we need a placeholder to prevent the suite from failing automatically (when empty)
| }; | ||
|
|
||
| export function ErrorBoundary() { | ||
| const error = useRouteError() as RouteError; |
There was a problem hiding this comment.
It's worth reading the sentry docs about integration with react-router: https://docs.sentry.io/platforms/javascript/guides/react/features/react-router/
I'm not sure what the differences would be but it looks like there are some pre-packaged wrappers that could be helpful.
Between that and the ErrorBoundary component that Sentry exports (https://docs.sentry.io/platforms/javascript/guides/react/features/error-boundary/), i wonder if we need to create our own anymore (I don't know if there was a good reason for not using it in the first place and rolling out our own instead)
Can be kept for a follow-up PR.
There was a problem hiding this comment.
I agree, Lets refactor this Error Boundary in the follow-up PR. We also have to rewrite the this error boundary.
| <Helmet> | ||
| <title>Error</title> | ||
| </Helmet> | ||
| <h2 className="page-title">Error Occured!</h2> |
There was a problem hiding this comment.
Shouldn't we display the errorMessage and a reload/home button in this section?
There was a problem hiding this comment.
Looking great @anshg1214 !
I deployed to tests again for testing, everything I tried is working fine.
Great job!
This PR migrates the User Dashboard and Explore pages to SPA