Change instance tabs into routes #1267
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
just-be-dev
changed the title
david-crespo
left a comment
david-crespo
left a comment
There was a problem hiding this comment.
Overall great, just some issues I'm not sure how to handle
app/routes.tsx
Outdated
| loader={CreateInstanceForm.loader} | ||
| handle={{ crumb: 'New instance' }} | ||
| /> | ||
| <Route path="instances/:instanceName" element={<Navigate to="storage" />} /> |
There was a problem hiding this comment.
These immediate navigates are trouble, unfortunately. Notice that when you click a link to the instance from the instances list, it doesn't actually land on instances/my-inst/storage.
I have a bunch of notes written up about this, but basically they don't work and neither does returning a redirect from a loader, though they break in different ways. This is why I changed the project list pages to link directly to /projects/my-proj/instances.
There was a problem hiding this comment.
Oh also if it's not a replace and it did work, it would have a broken back button — every back() would navigate forward again.
There was a problem hiding this comment.
Would it be possible for us to do a redirect in the loader?
There was a problem hiding this comment.
I mentioned that in the first comment. I did try it at one point, and it didn't behave the way I wanted, but I don't remember exactly how. Something about when the loader runs, what screen you're sitting on while it's running. Basically nothing I tried worked well for all of the following:
- Clicking a link to
/instance/my-inst - Clicking a link to
/instance/my-inst/storage - Loading
/instance/my-instdirectly - Loading
/instance/my-inst/storagedirectly
There was a problem hiding this comment.
D'oh, so you did. I'd love to see your notes on this. I assume loaders would be the right way to do it if they were actually running on the server.
There was a problem hiding this comment.
I solved this mostly in the same way in 57854aa. I'd like your input on the instancePage path part.
There was a problem hiding this comment.
Found some of my notes, they're somewhat helpful. Sounds like I was getting this:
- Click
/orgs/:orgNamelink - Land on
/orgs/:orgNameroute, which is blank because it has no content besides the redirect - Sit on that blank screen while the loader for
/orgs/:orgName/projectsruns - Finally land on
/orgs/:orgName/projectsas desired
I believe that's true with the <Navigate />, not returning the redirect from the loader. But if you do the redirect in the loader, I don't think it can be a replace, which means it makes a history entry, which is bad because the back button doesn't work, it redirect loops.
Running into another race condition with this config.
<Route path="/orgs/:orgName" element={<Navigate to="projects" replace />} loader={ProjectsPage.loader} /> <Route path="orgs" errorElement={<RouterDataErrorBoundary />}> <Route path=":orgName" handle={{ crumb: orgCrumb }}> <Route path="projects" handle={{ crumb: 'Projects' }}> <Route element={<OrgLayout />}> <Route index element={<ProjectsPage />} loader={ProjectsPage.loader} />The loader on the redirect is meant to avoid the blank flash while it sits on
/orgs/:orgNamewhile the loader onprojectsruns. But adding the loader causes the replace to just disappear somehow. When I click on an org from/orgs, instead of landing/orgs/maze-war/projects, I land on/orgs/maze-waras a replace, which means the back button takes me back to wherever I was before/orgs. Commenting out the loader brings back the flash as expected, but the replace now works. So somehow, the loader running breaks the navigation. Passing() => {}as the loader also causes the problem, so that’s a clue. Putting the loader on the redirect route but taking it off the target makes us land on the right route, but again as a replace directly from/orgs. Somehow the nav to/orgs/maze-waris not registering at all to be replaced.
| <div className={cn('ox-tabs', { 'full-width': fullWidth })}> | ||
| <div role="tablist" className="ox-tabs-list flex"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
wonder if this should be a <nav>
There was a problem hiding this comment.
Oh and maybe it's not really a role=tablist? From a control POV it's really just a nav menu styled like tabs unless we implement arrow key behavior. Overall kind of unsure about how to think about this from an a11y perspective.
There was a problem hiding this comment.
I think the accessibility behavior here should be tabs. Given where it's at in the flow and the fact that it's tied with the panel I think that's what makes the most sense here. I've updated the route tabs to be closer to the proper aria definition of tabs. The only thing that I know that I'm missing is the aria-describedby for the panel to tab relationship. I'm not home right now so I can't really thoroughly test it with my typical screen reader, but keyboard nav works now mostly as you'd expect.
|
Noticed a couple of small issues:
Looks like the label is vertically centered in the original but not in the route tabs.
Actually there's something else going on with the inner div too, it's too tall or something. (Raise color is very subtle, that might be worth tweaking, out of scope for this PR.)
|
|
We chatted about this in matrix, but 2 is mostly a weird FF behavior on OSX where by default links aren't tabbable. You can change your settings to make it work, but otherwise you're SOL.
Read more about it here: http://kb.mozillazine.org/Accessibility.tabfocus |
|
Focus styles for tab panel is fixed:
|
As discussed briefly in chat today, I believe that tabbed interfaces that function as primary navigation should be routes. This was reinforced for me today when I was trying to implement the new forms for the
network-interface-edit. Currently that lives on the/instances/:instanceroute and is only represented in the URI via a query param. The new form pattern establishes using a loader to populate data for edit modals and that pattern wouldn't work as well with our current route structure.Generally I just think these resources (disks, nics, metrics, serial console) should be treated as first class in our routing structure rather than be relegated to one off query params.
Changes
RouteTabscomponent which is essentially a bare tab structure component with tabs as navlinks and the panel containing an outletRouteTabsto look the same as theTabscomponent with no styles of its own.