feat(Link): add react-router link integration #294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for cfpb-design-system-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
billhimmelsbach
changed the title
billhimmelsbach
requested review from
meissadia and
shindigira
as code owners
meissadia
reviewed
Jan 30, 2024
| <BrowserRouter> | ||
| <p> | ||
| <Link href='/#' isRouterLink> | ||
| Link using React Router Link |
There was a problem hiding this comment.
Note: after writing out everything below I realized that the fact that it is a CFPB styled Link is important to the name, so you can ignore the rest of this comment 😬
The naming feels a bit awkward. Since it's ultimately returning a RouterLink it would be easier to simply call this React Router Link.
That said, we've been adjusting naming of variants in other components (ex. Instead of "Checkboxes -> Large target area checkbox" using "Checkboxes -> Large target area" to leverage the context that implies that everything under the "Checkboxes" heading is a "checkbox") and the names for Link could probably use some improvement as well. So this is probably fine as-is and can be re-evaluated in the next round of naming reviews.
meissadia
approved these changes
Jan 30, 2024
|
Thanks @meissadia! I appreciate the thinking about the naming. |
billhimmelsbach
added a commit
to cfpb/sbl-frontend
that referenced
this pull request
Jan 30, 2024
#201) This integrates cfpb/design-system-react#294 into the app, adding a little composition to the DSR component to automatically switch from normal anchor links to `react-router` links depending on the `href`. I wanted to make the transition as easy as possible: it's the same props as the DSR `Link`. As long as you're importing the Link from the app instead of the `DSR`, you don't have to worry about figuring out which links should have `isRouterLink` or not. If we like this approach, I could see us raising this up into the DSR as a "SBL-specific component" in a later PR. **Note: don't merge this in until cfpb/design-system-react#294 is merged and [this line](https://github.com/cfpb/sbl-frontend/compare/198-integrate-react-router-links?expand=1#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R37) that temporarily updates the DSR is reverted** ## Changes - adds new `Link` and `ListLink` components to the App that switch between `a` and `RouterLink` depending on what `href` is given - updates all instances of `Link` / `ListLink` across the app to use the new components - updates usage of plain `<a>` anchors to use `Link` - dedupes `react-router-dom` in the vite config that works with this change in the DSR to allow for `react-router` enabled `Links` to work within the same instance of `react-router` (what a pain this was) ## How to test this PR 1. Do external and internal links still work? 2. Do internal links (particularly breadcrumbs) seem faster with less page jank? ## Screenshots https://github.com/cfpb/sbl-frontend/assets/19983248/a239a808-b200-4244-b910-b6f9141ba244
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: cfpb/sbl-frontend#117
Well this took some experimentation, but we got there. Allows
Linkto use aisRouterLinkprop to convert them toreact-routerlinks with minimal hassle.Changes
react-router-domas an external dependency in the vite config, sincereact-routerneeds its links to share the same instance of the library to work. Works in concert with this change insbl-frontend. What a pain this was. (Also, I added a maybe missingreact-domfrom theoptimizeDeps.excludearray for good measure)isRouterLinkprop that switchesLinkto usingreact-routerLinksisRouterLinkprop called "Link using React Router Link"How to test this PR
isRouterLinkstory "Link using React Router Link" render without errors?Screenshots
Notes
NavLinkoption, but I don't think we have a use case for it since we have some similar logic built out already. I made a ticket for it, in case we want to add it in.