feat(ui-core): add PageShell application shell component - #33581
reethika-pd wants to merge 2 commits into
Conversation
Adds a reusable application shell to openmetadata-ui-core-components: a nav card and a page canvas on a viewport ground, with each region on its own surface token so the depth hierarchy holds in light and dark without per-theme markup. Compound API: PageShell + .Nav / .MainNav / .SubNav / .NavItem / .Canvas / .CanvasHeader / .CanvasBody. Notable behaviour: - Sub nav has three states. `hidden` keeps the column mounted at zero width so it animates out rather than popping, and is taken out of the tab order and the a11y tree while hidden — a zero-width column is otherwise still focusable. Its visibility control lives in the main nav, the one column always on screen, so `hidden` stays reversible. - Nav items drop their label to icon-only when their own column collapses, keeping the label as the accessible name and as a tooltip. Each column collapses independently. - Region edges are outlines, not borders, per the library's border rule: these containers are not focusable so their outline is free, and an outline costs no layout, leaving each region's padding as the real inset. - Nav state is semi-controlled: pass `subNav` / `isMainNavCollapsed` to control it, or `defaultSubNav` / `defaultMainNavCollapsed` to let the shell own it. - Accessible names come from a required `labels` prop rather than built-in strings, matching how PageLayout takes `aria-label` from the caller, so the consuming app keeps ownership of its i18n. globals.css gains the shell surface, border and outline tokens across all three layers the file requires — semantic --color-*, the property-namespace alias that generates the utility, and the --tw-*-color-* mirror inside .dark-mode. The third layer is not optional: Tailwind declares --tw-* with @Property { inherits: false }, so without it dark mode renders light values. The canvas header's light-theme tint is applied by a .shell-header-tint class rather than a token, because an unrecognised theme namespace is emitted as --tw-gradient-* and cannot be re-pointed per theme. The Storybook preview gains a `fullBleed` story parameter (the shell owns its outer padding and must reach the canvas edge) and an explicit sidebar sort order. Spacing, verified against the design spec in the browser: 6px viewport padding, 8px nav-to-canvas gap, 8px canvas padding, 16/20 canvas header inset, 16px header-to-body gap. Tests: 11 cases covering landmarks, per-column collapse, the hidden-state contract, controlled/uncontrolled behaviour, and the icon-only fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rence Read from the reference frame: the page canvas is a 900 panel whose edge is 800 — one step lighter than the 700 card edges inside it — so the outer panel reads softer than the cards. Everything else in dark already matched: app ground 950, nav card and canvas 900, main nav inset and cards 800, card edges 700, on the library's existing gray ramp. Dark token: border-shell-canvas 700 -> 800 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const isHidden = subNav === 'hidden'; | ||
| const isExpanded = subNav === 'expanded'; | ||
| const width = isHidden ? 0 : isExpanded ? navWidth : railWidth; | ||
| const subNavColumn = useMemo( | ||
| () => ({ isCollapsed: !isExpanded }), | ||
| [isExpanded] | ||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
| {...props} | ||
| aria-hidden={isHidden} | ||
| className={cx( | ||
| 'tw:flex tw:h-full tw:shrink-0 tw:flex-col tw:overflow-hidden', | ||
| TRANSITION, |
There was a problem hiding this comment.
⚠️ Edge Case: Hidden sub-nav content stays focusable and Tab-reachable
When subNav === 'hidden' the column collapses to width: 0 with overflow: hidden, and only the CollapseToggle is pulled from the tab order via isFocusable={!isHidden}. The sub-nav children (the consuming app's NavItem buttons/anchors) are not touched — overflow:hidden at zero width does not remove focus, so a keyboard user can still Tab into the invisible column, exactly the stranding the component's own comment says it prevents. Worse, those focusable descendants sit inside aria-hidden={true}, which is the aria-hidden-focus anti-pattern (focusable element in a hidden subtree). The unit test only asserts the toggle's tabindex, so it misses the child items. Apply inert (or set hidden) to the whole column when hidden so the content and toggle are all removed from the tab order and a11y tree together.
Mark the whole hidden column inert so its children and toggle are removed from the tab order and a11y tree, not just the toggle.:
<div
{...props}
aria-hidden={isHidden}
inert={isHidden ? true : undefined}
className={cx(
'tw:flex tw:h-full tw:shrink-0 tw:flex-col tw:overflow-hidden',
TRANSITION,
className
)}
data-region="sub-nav"
data-state={subNav}
style={{ width }}>
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
1 participant
Describe your changes:
Fixes #
Adds a reusable
PageShellapplication-shell component toopenmetadata-ui-core-components: a nav card and a page canvas floating on a viewport ground, with each region on its own surface token so the depth hierarchy holds in light and dark without per-theme markup.Compound API:
PageShell+.Nav/.MainNav/.SubNav/.NavItem/.Canvas/.CanvasHeader/.CanvasBody.Why: pages in the new design system need a consistent outer frame — nav columns that collapse to a rail, a sub-nav that can be hidden entirely, and a canvas with a header/body split — and today each page would have to rebuild that from raw layout. This puts the frame, its spacing spec, and its tokens in one place.
Type of change:
High-level design:
Structure. Regions are compound children that read shared state from a
PageShellContext;MainNav/SubNavadditionally provide a per-column context so aNavItemknows whether its own column is collapsed (the two collapse independently). Nav state is semi-controlled:subNav/isMainNavCollapsedfor controlled use,defaultSubNav/defaultMainNavCollapsedotherwise, with change callbacks.Three sub-nav states, not two. A page with no second level needs the column gone, not narrow.
hiddenkeeps the column mounted at zero width so it animates out rather than popping, and while hidden it isaria-hiddenwith its toggle attabIndex={-1}— a zero-width column is otherwise still focusable and strands keyboard users. Its visibility control lives in the main nav (always on screen) sohiddenstays reversible; a restored sub nav returns to the width it had.Nav items go icon-only when collapsed. The label leaves the layout rather than truncating (a 44px rail can't show a readable word), and comes back as
aria-label+titleso the item keeps its accessible name.Edges are outlines, not borders, per the library's border rule: these containers aren't focusable so their outline is free, and an outline costs no layout — a border sits inside the box and adds itself to every region's padding.
No strings in the library. Accessible names come from a required
labelsprop, matching howPageLayouttakesaria-labelfrom the caller, so the consuming app keeps ownership of its i18n.Tokens.
globals.cssgains the shell surface/border/outline tokens across the three layers the file requires — semantic--color-*, the property-namespace alias that generates thetw:*-shell-*utility, and the--tw-*-color-*mirror inside.dark-mode. The third is not optional: Tailwind declares--tw-*with@property { inherits: false }, so without it dark mode renders light values. The canvas header's light-theme tint is a.shell-header-tintclass rather than a token because an unrecognised theme namespace is emitted as--tw-gradient-*and cannot be re-pointed per theme. Text uses the library's existing scale; there are no shell text tokens. Surfaces step white / off-white in light and 950 → 900 → 800 in dark on the existing gray ramp; the dark canvas edge is 800, one step lighter than the 700 card edges inside it.Spacing (verified in the browser against the design spec): 6px viewport padding, 8px nav→canvas gap, 8px canvas padding, 16/20 canvas-header inset, 16px header→body gap; nav columns 180px expanded / 44px rail.
Alternatives considered. A single boolean for the sub nav (rejected: can't express "gone"); borders for region edges (rejected: added 0.5px to every inset and broke the spec); built-in English labels (rejected: fails
check-i18n-alland would need 19 translations for six strings).Storybook. The preview gains a
fullBleedstory parameter (the shell owns its outer padding and must reach the canvas edge) and an explicit sidebar sort order. Stories live underTesting/PageShell.Tests:
Use cases covered
complementarylandmark; canvas body renders asmainaria-hidden, and its toggle leaves the tab orderonSubNavChangeand does not move without the prop changingaria-label+title) when their column collapses, and each column collapses independentlyNavItemrenders an anchor whenhrefis setPageShellthrows a clear errorUnit tests
openmetadata-ui-core-components/src/main/resources/ui/src/components/application/page-shell/page-shell.test.tsx(11 tests, Vitest + Testing Library)Backend integration tests
Ingestion integration tests
Playwright (UI) tests
ui-core-componentslibrary component with no page inopenmetadata-uiconsuming it yet; behaviour is covered by the component tests above and the Storybook stories.Manual testing performed
cd openmetadata-ui-core-components/src/main/resources/ui && yarn storybook, opened Testing → PageShellyarn type-check,eslint,prettier --check,yarn check-i18n-keys— all cleanUI screen recording / screenshots:
Storybook only — no
openmetadata-uipage changes. Stories:Testing/PageShell→ Default, Without Sub Nav, Collapsed.Checklist:
Fixes <issue-number>: <short explanation>— pending an issue numberFixes #<issue-number>above — pendingsrc/styles/globals.cssis intentionally not Prettier-formatted (it fails--checkonmain). Please don't format it in review — the diff here is purely additive (+139 / −0 for tokens, plus one dark value).🤖 Generated with Claude Code