-
Notifications
You must be signed in to change notification settings - Fork 4
Use wouter for routing
#969
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
Conversation
| <div className="sticky top-0 z-4 h-16 flex items-center bg-slate-0 border-b"> | ||
| <div | ||
| className="sticky top-0 z-4 h-16 flex items-center bg-slate-0 border-b" | ||
| id="page-header" |
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 the page's sticky header. Just giving it an ID so I can query for it in the PlaygroundApp's navigation/scroll handling useEffect hook.
| function Link({ children, href }: LinkProps) { | ||
| return ( | ||
| <RouteLink href={href}> | ||
| <UILink underline="always">{children}</UILink> | ||
| </RouteLink> | ||
| ); | ||
| } |
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.
So many things named Link, heh.
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.
We should call this component LinkLink 😝
| import type { PlaygroundRoute } from '../routes'; | ||
| import Library from './Library'; | ||
|
|
||
| /** |
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.
No meaningful changes to NavHeader, NavSection or NavList: just extracting them so that they are not inside of the PlaygroundApp component function.
| /** | ||
| * A single navigation link | ||
| */ | ||
| function NavLink({ route }: { route: PlaygroundRoute }) { |
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.
But there are (obviously) changes here to use the wouter API.
Ensure that when navigation to a new page occurs, scroll is reset to the top of the page, unless there is a fragment in the URL. When there is a fragment, scroll the element indicated by the fragment to the top of the page.
Add a `LibraryLink` component that wraps the package's `Link` component for linking internally to other `wouter`-routed pattern-library pages.
9e9acd5 to
b701a0b
Compare
| <nav id="nav" className="pt-8 pb-16 space-y-4 mr-4"> | ||
| <NavHeader>Foundations</NavHeader> | ||
| <NavList routes={getRoutes('foundations')} /> | ||
| <Router base={baseURL}> |
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.
NB: Except for wrapping the whole thing in a Router, there are no wouter-specific changes here except where noted.
| * navigation, with no link. | ||
| */ | ||
| route?: RegExp | string; | ||
| route?: string; |
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.
Adjusted to work with wouter types. We only used RegExp in one route, which isn't needed anymore.
| <Switch> | ||
| {pageRoutes} | ||
| <Route> | ||
| <Library.Page title=":( Sorry"> | ||
| <h1 className="text-2xl">Page not found</h1> | ||
| </Library.Page> | ||
| </Route> | ||
| </Switch> |
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.
<Switch> is a wouter component that will render (only) the first matching <Route> inside of it.
Codecov Report
@@ Coverage Diff @@
## main #969 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 52 52
Lines 671 671
Branches 230 230
=========================================
Hits 671 671 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
acelaya
left a comment
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.
Nice one.
This PR replaces the custom routing we've been using in the pattern library with
wouter-preact, which we also use in thelmsproject. By usingwouter, I was also able to find a solution for fragment navigation. It is now possible to link between pattern-library pages, and to use links with hashes. The API for passing routes to the app has not changed (so, no breaking changes here).There's plenty of potential follow-up work here: establishing IDs (for hash-linking) on library heading elements, providing tables of contents for pages, making the new
<Library.Link>component support external links. But all for separate PRs!Testing
Run
make devon this branch and visit the pattern library. Ensure that:Fixes #557