-
Notifications
You must be signed in to change notification settings - Fork 16
Change instance tabs into routes #1267
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
aaa586c
Move instance tabs to routes
just-be-dev be722f8
Remove unused paths from pb
just-be-dev a212deb
Update path builder snapshots
just-be-dev 35cde4a
Add roles to route tabs; fix tab names
just-be-dev 88aad28
Merge branch 'main' into route-tabs
just-be-dev 59dc7f4
Fix navigation issues in tests
just-be-dev f500bc6
Remove serial console page for now
just-be-dev abf5e95
Merge branch 'main' into route-tabs
just-be-dev 0220001
Lazy load metrics tab
just-be-dev 74a03bd
Merge branch 'main' into route-tabs
just-be-dev 047dd93
Merge branch 'main' into route-tabs
just-be-dev 85b5d68
Merge branch 'main' into route-tabs
just-be-dev 1aadbff
Update props to be more explicit
just-be-dev 615229c
Correct instance tabs paths
just-be-dev 8f7c4f5
Update routetabs to be closer to full aria spec
just-be-dev 4a14b29
Add comment about describedby
just-be-dev bd8ae69
Implement wrap around behavior for arrows
just-be-dev 57854aa
Experimentally solve routing issue
just-be-dev c8fd325
Update route tests
just-be-dev e0c2026
fix e2e paths
just-be-dev bc426a5
Merge branch 'main' into route-tabs
just-be-dev ac1cb45
Move argument into options of useIsActivePath
just-be-dev 88bd3e9
Fix tab layout styles
just-be-dev df542a4
Merge branch 'main' into route-tabs
just-be-dev a0743a3
Fix tab panel focus ring
just-be-dev 782dead
Improve instancePage comment
just-be-dev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import cn from 'classnames' | ||
| import type { ReactNode } from 'react' | ||
| import { Link, Outlet } from 'react-router-dom' | ||
|
|
||
| import { useIsActivePath } from 'app/hooks/use-is-active-path' | ||
|
|
||
| const selectTab = (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| const target = e.target as HTMLDivElement | ||
| if (e.key === 'ArrowLeft') { | ||
| e.stopPropagation() | ||
| e.preventDefault() | ||
|
|
||
| const sibling = (target.previousSibling ?? | ||
| target.parentElement!.lastChild!) as HTMLDivElement | ||
|
|
||
| sibling.focus() | ||
| sibling.click() | ||
| } else if (e.key === 'ArrowRight') { | ||
| e.stopPropagation() | ||
| e.preventDefault() | ||
|
|
||
| const sibling = (target.nextSibling ?? | ||
| target.parentElement!.firstChild!) as HTMLDivElement | ||
|
|
||
| sibling.focus() | ||
| sibling.click() | ||
| } | ||
| } | ||
|
|
||
| export interface RouteTabsProps { | ||
| children: ReactNode | ||
| fullWidth?: boolean | ||
| } | ||
| export function RouteTabs({ children, fullWidth }: RouteTabsProps) { | ||
| return ( | ||
| <div className={cn('ox-tabs', { 'full-width': fullWidth })}> | ||
| {/* eslint-disable-next-line jsx-a11y/interactive-supports-focus */} | ||
| <div role="tablist" className="ox-tabs-list flex" onKeyDown={selectTab}> | ||
| {children} | ||
| </div> | ||
| {/* TODO: Add aria-describedby for active tab */} | ||
| <div className="ox-tabs-panel" role="tabpanel" tabIndex={0}> | ||
| <Outlet /> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export interface TabProps { | ||
| to: string | ||
| children: ReactNode | ||
| } | ||
| export const Tab = ({ to, children }: TabProps) => { | ||
| const isActive = useIsActivePath({ to }) | ||
| return ( | ||
| <Link | ||
| role="tab" | ||
| to={to} | ||
| className={cn('ox-tab', { 'is-selected': isActive })} | ||
| tabIndex={isActive ? 0 : -1} | ||
| aria-selected={isActive} | ||
| > | ||
| <div>{children}</div> | ||
| </Link> | ||
| ) | ||
| } | ||
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { useLocation, useResolvedPath } from 'react-router-dom' | ||
|
|
||
| interface ActivePathOptions { | ||
| to: string | ||
| end?: boolean | ||
| } | ||
| /** | ||
| * Returns true if the provided path is currently active. | ||
| * | ||
| * This implementation is based on logic from React Router's NavLink component. | ||
| * | ||
| * @see https://github.com/remix-run/react-router/blob/67f16e73603765158c63a27afb70d3a4b3e823d3/packages/react-router-dom/index.tsx#L448-L467 | ||
| * | ||
| * @param to The path to check | ||
| * @param options.end Ensure this path isn't matched as "active" when its descendant paths are matched. | ||
| */ | ||
| export const useIsActivePath = ({ to, end }: ActivePathOptions) => { | ||
| const path = useResolvedPath(to) | ||
| const location = useLocation() | ||
|
|
||
| const toPathname = path.pathname | ||
| const locationPathname = location.pathname | ||
|
|
||
| return ( | ||
| locationPathname === toPathname || | ||
| (!end && | ||
| locationPathname.startsWith(toPathname) && | ||
| locationPathname.charAt(toPathname.length) === '/') | ||
| ) | ||
| } |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
40 changes: 0 additions & 40 deletions
40
app/pages/project/instances/instance/SerialConsolePage.tsx
This file was deleted.
Oops, something went wrong.
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
wonder if this should be a
<nav>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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-describedbyfor 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.