-
Notifications
You must be signed in to change notification settings - Fork 129
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: [FC-0070] support SPA functionality #542
feat: [FC-0070] support SPA functionality #542
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:
Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
==========================================
- Coverage 70.55% 68.75% -1.81%
==========================================
Files 25 48 +23
Lines 360 432 +72
Branches 95 97 +2
==========================================
+ Hits 254 297 +43
- Misses 104 132 +28
- Partials 2 3 +1 ☔ View full report in Codecov by Sentry. |
Hey @PKulkoRaccoonGang, thank you for this contribution! It looks like the changes are ready for review (?), but let me know if you were planning to work on them some more. |
Hi @itsjeyd |
Sounds good @PKulkoRaccoonGang, thanks for the update. |
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.
I'm missing an explanation of why we're doing this complicated onNavigate prop injection and duplicating logic from react-router instead of simply using <Link>
.
src/studio-header/BrandNav.jsx
Outdated
}) => ( | ||
<a href={studioBaseUrl}> | ||
<Hyperlink destination={studioBaseUrl} onClick={(e) => navigateToUrl(e, studioBaseUrl, onNavigate)}> |
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 is OK, but isn't it much simpler and nicer to just use the <Link>
component from react-router-dom
?
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.
I'm missing an explanation of why we're doing this complicated onNavigate prop injection and duplicating logic from react-router instead of simply using .
The reason why it is not possible to use the react-router-dom functionality is the lack of a react-router-dom context, in the Authoring MFE the Header is outside the react-router context of the app. If you have any workarounds, I will be happy to implement them
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.
Are you sure? I thought the root app always provides RouterProvider
. And in that page you linked to, I can see the use of useLocation
from react-router which means the router context is available.
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.
Thanks to this PR, I realized that I wasn't the only one with the router context problem. Before adding react-router-dom
to peerDependencies
, the following error was displayed when using Link
. I don't understand why it works like that, but it does :)
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.
Glad you figured that out! Thanks, this seems to be working well, and it makes everything feel much faster :)
5ead432
to
0f9014b
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.
The code itself is great. I tested it and everything works beautifully. No more page refreshes, yay!
However, can you please make the commit a feat:
? It may not be a breaking change, but it does offer a new isNewHomePage
prop for the StudioHeader.
0f9014b
to
6e4945b
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.
Thank you!
🚨 Dependencies:
Description
This PR introduces several changes to the
studio-header
components to integratereact-router-dom
for client-side routing, along with corresponding updates to tests.Testing: