React Router v6 beta 6 #481
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/2tVbbP9Vv37aKQNSYw2kVukL9sgt |
| @@ -28,20 +28,20 @@ const App = () => ( | |||
| <Router> | |||
| <QuickMenu /> | |||
| <Routes> | |||
| <Route path="/" element={<Navigate to="/projects" replace={true} />} /> | |||
| <Route index element={<Navigate to="/projects" replace={true} />} /> | |||
There was a problem hiding this comment.
they added this index thing because of changes to the handling of absolute paths. I like it a lot better than path="/" anyway.
If you were using
<Route path="/">to indicate an index route, you can now use the new<Route index>prop to accomplish the same thing. Theindexprop makes it easy to scan a route config to find the index route. It also provides a guarantee that nobody will ever add children to that route.
https://github.com/remix-run/react-router/releases/tag/v6.0.0-beta.4
| <Route path="new" element={<ProjectCreatePage />} /> | ||
| </Route> | ||
|
|
||
| {/* PROJECT */} | ||
| <Route path="/projects/:projectName" element={<ProjectLayout />}> |
There was a problem hiding this comment.
this leading slash was fine but not necessary. plus once we nest this all under /orgs/cyberdyne it would have been necessary to remove it
| @@ -31,9 +31,11 @@ const projectPaths: Record<string, (s: string) => string> = { | |||
| 'Project access': (projectName) => `/projects/${projectName}/access`, | |||
| } | |||
|
|
|||
| // can't use useParams because QuickMenu is not rendered inside the route tree, so | |||
| // it does not have access to the current route | |||
There was a problem hiding this comment.
had to re-learn the hard way why I did this. next time I will not have to
There was a problem hiding this comment.
Future me will likely thank you for this too
| @@ -46,8 +47,12 @@ export const NavLinkItem = (props: { | |||
| <li> | |||
| <NavLink | |||
| to={props.to} | |||
| className="flex items-center p-1 hover:bg-gray-400 svg:mr-2 svg:text-gray-300" | |||
| activeClassName="text-white svg:!text-green-500" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
This seemed like an odd API choice at first but it makes sense after looking at the root issue that inspired it: remix-run/react-router#7194. It's particularly helpful in our case given that we're using tailwind and could run afoul of that issue.
david-crespo
force-pushed
the
react-router-v6-beta6
branch
from
170bac9
to
d70652f
Compare
|
Preview will be deployed at https://console-git-react-router-v6-beta6.internal.oxide.computer |
| @@ -21,7 +21,6 @@ const ProjectList = (props: { className?: string }) => { | |||
| <NavLink | |||
| className="inline-flex w-full p-1" | |||
| to={`/projects/${p.name}`} | |||
| activeClassName="text-white" | |||
There was a problem hiding this comment.
Was this just not needed or does it still need to be converted over?
There was a problem hiding this comment.
No longer needed because we never stay on the org layout after a project has been selected.
|
Refreshingly straight forward. The only proper question I have is about the |
Only small changes needed because of #465. I've had this change cooking since before I was on leave, just want to wrap it up. v6 should be coming out of beta in the next week or two:
We're not using relative links "up" the route tree or
useBlocker(which they're apparently taking out) so we should be pretty much good for v6 stable after this.