feat(SidenavBar): full specs implementation - #1673
Conversation
|
Size stats
|
|
Deploy preview for mistica-web ready!
Deployed with vercel-action |
|
Accessibility report ℹ️ You can run this locally by executing |
brtbrt
changed the title
There was a problem hiding this comment.
Pull request overview
Adds the new SidenavBar component (plus SidenavLayout) and supporting infrastructure (shared navigation modules, stories/tests, generated tokens tooling) to implement the sidenav specs across desktop/mobile/double-panel variants.
Changes:
- Introduces
src/sidenav-bar/(implementation, layout, motion hook, stories) and exports it from the package entrypoint. - Adds new i18n text tokens for SidenavBar accessibility labels and updates some acceptance/unit test expectations.
- Improves design-token generation outputs (banners + stable prettier config resolution) and updates storybook/jest/tooling config to support the new “screaming architecture” layout.
Reviewed changes
Copilot reviewed 33 out of 2332 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/text-tokens.tsx | Adds new SidenavBar text tokens (expand/collapse/submenu/landmark). |
| src/skins/vivo.tsx | Generated skin update + adds generated-file banner. |
| src/skins/vivo-evolution.tsx | Generated skin update (text presets/padding) + generated-file banner. |
| src/skins/types/colors.tsx | Adds generated-file banner for generated type. |
| src/skins/telefonica.tsx | Generated skin update + adds generated-file banner. |
| src/skins/o2.tsx | Generated skin update + adds generated-file banner. |
| src/skins/movistar.tsx | Generated skin update + adds generated-file banner. |
| src/skins/esimflag.tsx | Generated skin update + adds generated-file banner. |
| src/skins/blau.tsx | Generated skin update + adds generated-file banner. |
| src/sidenav-bar/stories/bar/sidenav-story-page.tsx | Adds a story wrapper page using SidenavLayout. |
| src/sidenav-bar/stories/bar/sidenav-bar-right-slots-story.tsx | Adds story showcasing rightSlot usage. |
| src/sidenav-bar/stories/bar/sidenav-bar-nested-story.tsx | Adds story showcasing nested items. |
| src/sidenav-bar/sidenav-section.tsx | Adds SidenavSection component. |
| src/sidenav-bar/sidenav-motion.tsx | Adds reduced-motion hook used by SidenavBar JS-driven timing. |
| src/sidenav-bar/sidenav-layout.tsx | Adds SidenavLayout component. |
| src/sidenav-bar/sidenav-layout.css.ts | Adds layout styles (sticky rail + responsive stacking). |
| src/sidenav-bar/index.tsx | Adds SidenavBar public exports from the sidenav-bar module. |
| src/portal.tsx | Updates portal doc link to react.dev reference. |
| src/navigation-types.ts | Adds shared navigation interactive prop types (href/to/onPress unions). |
| src/index.tsx | Re-exports SidenavBar/SidenavLayout (and types) from the package entrypoint. |
| src/generated/mistica-icons/icon-text-size-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-terminal-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-terminal-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-terminal-console-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-subtract-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-stop-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-stop-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-stop-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-software-window-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-forward-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-forward-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-forward-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-back-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-back-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-skip-back-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-robot-vacuum-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-ring-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-ring-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-ring-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-recording-circle-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-low-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-low-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-low-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-highest-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-high-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-high-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-rank-high-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-panel-expand-regular.tsx | Adds new generated panel-expand icon. |
| src/generated/mistica-icons/icon-panel-expand-light.tsx | Adds new generated panel-expand icon. |
| src/generated/mistica-icons/icon-panel-expand-filled.tsx | Adds new generated panel-expand icon. |
| src/generated/mistica-icons/icon-panel-collapse-regular.tsx | Adds new generated panel-collapse icon. |
| src/generated/mistica-icons/icon-panel-collapse-light.tsx | Adds new generated panel-collapse icon. |
| src/generated/mistica-icons/icon-panel-collapse-filled.tsx | Adds new generated panel-collapse icon. |
| src/generated/mistica-icons/icon-message-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-meatball-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-meatball-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-lamp-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-lamp-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-lamp-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-kebab-menu-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-information-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-file-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-equal-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-equal-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-equal-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-download-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-chevron-left-double-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-chevron-left-double-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-check-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-battery-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-archive-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-add-more-filled.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-add-bolton-regular.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-add-bolton-light.tsx | Generated icon formatting update. |
| src/generated/mistica-icons/icon-add-bolton-filled.tsx | Generated icon formatting update. |
| src/boxed.tsx | Extracts/exports shouldShowBoxedBorder helper for reuse. |
| src/acceptance_tests/pagination-acceptance-test.tsx | Adjusts aria-current assertions for Pagination acceptance test. |
| packages/generate-design-tokens/index.js | Adds generated-file banner + fixes prettier config resolution by using file paths. |
| package.json | Adds scripts (generate-design-tokens, import-mistica-icons, browse) and updates lint-staged patterns. |
| mistica-web.iml | Simplifies IntelliJ module content definition. |
| jest.unit.config.js | Excludes colocated stories/ from unit-test match patterns. |
| css/vivo.css | Generated CSS update + adds generated-file banner. |
| css/vivo-evolution.css | Generated CSS update (text tokens) + adds generated-file banner. |
| css/telefonica.css | Generated CSS update + adds generated-file banner. |
| css/o2.css | Generated CSS update + adds generated-file banner. |
| css/movistar.css | Generated CSS update + adds generated-file banner. |
| css/mistica-common.css | Generated CSS update + adds generated-file banner. |
| css/esimflag.css | Generated CSS update + adds generated-file banner. |
| css/blau.css | Generated CSS update + adds generated-file banner. |
| agents/README.md | Adds documentation for agent skills directory layout and symlink strategy. |
| AGENTS.md | Expands agent/repo conventions and workflow guidance. |
| .storybook/preview.tsx | Fixes event handler registration/cleanup + improves controls UI. |
| .storybook/main.ts | Includes new src/**/stories/**/*-story.tsx pattern. |
| .gitignore | Ignores local spec cache directory and Claude local settings. |
| .eslintrc.yml | Adds sidenav-bar-specific restricted import rules. |
| .claude/settings.json | Adds Claude hook to confirm before git commit* commands. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 'use client'; | ||
| import * as React from 'react'; | ||
|
|
||
| const REDUCED_MOTION_QUERY = '(prefers-reduced-motion)'; |
| // TODO WIP es/de/pt are drafts pending official localization sign-off; en is final. | ||
| export const sidenavExpand: TextToken = { | ||
| es: 'Expandir navegación', | ||
| en: 'Expand navigation', | ||
| de: 'Navigation ausklappen', |
Marcosld
left a comment
Marcosld
left a comment
There was a problem hiding this comment.
I'd split the review as you proposed to ease the review process
Sidenav component, stories, unit/acceptance/screenshot tests, shared navigation extraction (navigation-shared, mobile-navigation-menu), colocated-story wiring, eslint import rule, and the @generated token-banner tooling. Rebased onto the skins/icons update; AI harness lives on mbertamini/1592-ai-harness.
25eefa6 to
f805683
Compare
…o mbertamini/1592-sidenav-bar
| /** Whether user can toggle collapsed state. @default true */ | ||
| collapsible?: true; | ||
| /** Paints a custom collapse action in the header, instead of the default icon button. */ | ||
| renderCollapseAction?: (props: SidenavCollapseActionRenderProps) => React.ReactNode; |
There was a problem hiding this comment.
I think we don't need this prop, because we already have a header slot + controlled mode for those who want to render a custom controller, right?
There was a problem hiding this comment.
this is to replace the collapsing button the exact space it would occupy by default. the rest is to render elsewhere actually. I'd keep it
There was a problem hiding this comment.
Hmm, I think we can replicate exactly what renderCollapseAction does by just using the headerSlot. In that case we are adding unnecessary complexity to an already complex component and I would remove it.
There was a problem hiding this comment.
well, not really, because slot has different vertical spacings. you cannot replicate this, otherwide it would be useless, as you said.
let's say what @Telefonica/mistica-design say. if they want both, it's not such a big problem. this is really not what complicates the code. if they agree, we'll remove it and simplify the thing
There was a problem hiding this comment.
Oh I see! Can we just simplify and make the slot margin to top be same as the logo when there's no logo and current 32px when there's logo? Removing a prop is always simplifying the thing
There was a problem hiding this comment.
you don't get me: it does simplifies it, but it's not what makes the codebase tangled in this very case. again: designers? what's your say
There was a problem hiding this comment.
I mean simplifying from an user point of view that has less props to work with
|
|
||
| const headerObserver = new IntersectionObserver( | ||
| ([entry]) => { | ||
| setShowHeaderDivider(!entry.isIntersecting); |
There was a problem hiding this comment.
Perhaps we should rename these to something related to scroll so it is easier to follow?
There was a problem hiding this comment.
Mmmm I cannot make this work... what is it for? Perhaps we should let the slot manage its own scroll?
There was a problem hiding this comment.
Mmmm I cannot make this work
I don't understand this.
what is it for?
divider while scrolling content! I don't know how else describing this 🤔
There was a problem hiding this comment.
I'm just asking what is this for... can you paste a playroom snippet?
There was a problem hiding this comment.
Ohh I see. The one for the footer doesn't seem to be working, also perhaps we should implement something more similar to the fixed footer (perhaps reuse code ?), because with your implementation shadow disappears weirdly when the content bounces:
Screen.Recording.2026-08-27.at.16.20.35.mov
There was a problem hiding this comment.
the footer is not in the specs. I asked them to address it in a call, but I don't see it. maybe they don't want it for a good reason? we can iterate it.
bouncing: I'll try fix it, I guess we should change the place it's rendered and it could take me a lot of time without AI
There was a problem hiding this comment.
From reading those lines I understood it should behave the same but maybe I'm mistaken @Telefonica/mistica-design ?
There was a problem hiding this comment.
@Marcosld what are you using? I see it "behaving well" on firefox + safari + chromium
demo-scrolling.mov
There was a problem hiding this comment.
:O I am using edge... but it also happens in chrome for me
| }, []); | ||
|
|
||
| // The rail is travelling between its two widths. See `columnsWhileMoving`. | ||
| const isMoving = collapsed !== collapsedSettled; |
There was a problem hiding this comment.
Why do we need this logic?
There was a problem hiding this comment.
long story short: the CSStransition's events are not enough, we do need this to keep things well rendered while animating things
| const doublePanelItem = | ||
| doublePanel && panelOpenForItemId && sections | ||
| ? findFirstLevelItem(sections, panelOpenForItemId) | ||
| : undefined; | ||
| const doublePanelChildren = doublePanelItem?.children; | ||
| const isDoublePanelOpen = Boolean(doublePanelChildren?.length); | ||
|
|
||
| if (isDoublePanelOpen && doublePanelItem && doublePanelChildren) { | ||
| lastDoublePanelContentRef.current = {label: doublePanelItem.label, children: doublePanelChildren}; | ||
| } | ||
| const doublePanelContent = lastDoublePanelContentRef.current; |
There was a problem hiding this comment.
Animation concerns. The double panel uses a CSSTransition with unmountOnExit. So the panel does not vanish when you close it. it slides away over the close duration, and it still renders during that slide.
My own rule of thumb in general: derived or cached values that never drive the screen by themselves go in a ref, but values that the screen must follow go in a state. (and thank you for reading this non-ai generated lengthy thing!)
in detail the sequence is:
- The user closes the column, so panelOpenForItemId becomes null.
- doublePanelItem and doublePanelChildren derive from that state, so both become undefined at once
- The CsSTransition does not unmount the column at once. it slides to zero within a defined time
- BUT During that slide, the column still renders! Without the ref, it would have no label and no children, so it would slide away as an empty band.
So, while the column is open, the ref acts like a snapshot of the current label and children. When the column closes, the render reads lastDoublePanelContentRef.current and paints the stale content until the movement ends and unmountOnExit removes the node.
I can see you already asking: so why not considering a state? Well I've considered it (and asked the AI the same actually). Short answer: this saves useless renders. A state would store the same value and schedule one extra render for nothing. The write during the render is safe because it only caches what the current render already shows.
There was a problem hiding this comment.
I think having a simple code justifies having extra renders, few extra renders won't impact performance. This code along with previousSelection state var is a smell to me, something looks to be off. I think not having a panelOpen state var is the problem. We should have a panelOpenItemId state var and a panelOpen state var. That way we can toggle panel with panelOpen while just keeping the panelOpenItemId set (thus not needing the ref).
There was a problem hiding this comment.
This kind of defeats the point of agentic coding: getting good code for the same effort. A more semantic const name might've made the actual purpose obvious. But if you'd rather add the extra renders, sure, let's go with it.
| onSelectedItemIdChange?: (id: string | null) => void; | ||
| }; | ||
|
|
||
| const SidenavBarContext = React.createContext<SidenavBarContextValue>({ |
There was a problem hiding this comment.
Do we need these contexts? Can we just pass props down?
There was a problem hiding this comment.
props drill hell, I'd be stick with this
There was a problem hiding this comment.
I think the solution is overcomplicated, the sidenav should render its items and just pass the prop 1 boundary down
There was a problem hiding this comment.
nice to have, but I wouldn't give high priority right now. I'll give priority to API compliance and correctness of the behavior. we can iterate later
There was a problem hiding this comment.
Mmm not nice to have, I would simplify this so next person that needs to makes changes can understand it in less time (or even an AI)
There was a problem hiding this comment.
again, not against it, I can give it a thought (and I need to analyse this, because I think it's not simply a one level prop to pass, but I can be wrong. I need to see the full picture in my head, which I currently don't have on this), but it takes time and we do got a deadline, which is expiring today. prioritizing things is key in this case. that's why I prefer iterate later, or as a one of the lastest thing to do in the list!
There was a problem hiding this comment.
I'll keep this converstion open and if I got time for this I'll give it some love!
brtbrt
left a comment
brtbrt
left a comment
There was a problem hiding this comment.
answered and/or fixed the pending comments
|
Screenshot tests report ✔️ All passing |
There was a problem hiding this comment.
this comment is about technical things from a designer, just take it with puntillas
but the usage of <SidenavLayout.Sidenav> is really strange for my experience using Mística in Playroom with other types of elements. In my mind, this usage is quite out of the conventions (maybe, I'm full wrong, but I need to say haha)
There was a problem hiding this comment.
ok! what would you expect? write down a draft/idea here and let's adjust the DX. I think marcos also has some doubts about this API
There was a problem hiding this comment.
I've tried to figure out what you expect by looking at the other "layouty" components. let me know if you see a better fit
| }; | ||
|
|
||
| export default { | ||
| title: 'Components/SidenavBar/Layout', |
There was a problem hiding this comment.
maybe is better to move this story to the layouts section? - Layouts/SidenavBar/
Like the others layouts 🤔
https://mistica-j7kpk4qd3-flows-projects-65bb050e.vercel.app/?path=/story/layout-responsive-layout--default
There was a problem hiding this comment.
meh, I wouldn't do that. this is a side navbar related thing that you cannot use as a general layout. the rest of layouts can (and you can use the sidenav bar in it)
|
|
||
| const headerObserver = new IntersectionObserver( | ||
| ([entry]) => { | ||
| setShowHeaderDivider(!entry.isIntersecting); |
There was a problem hiding this comment.
From reading those lines I understood it should behave the same but maybe I'm mistaken @Telefonica/mistica-design ?
| <div | ||
| className={classnames(styles.mainColumn, { | ||
| [styles.columnSeparator[normalizedVariant]]: | ||
| isDoublePanelOpen || isDoublePanelMounted, |
There was a problem hiding this comment.
isDoublePanelMounted should be enough right?
There was a problem hiding this comment.
almost yes, but for animation reasons we must keep both
There was a problem hiding this comment.
could you elaborate, please?
There was a problem hiding this comment.
long story short: no, isDoublePanelMounted is not enough when closing it
There was a problem hiding this comment.
I think it is for when the panel is appearing instead, but as onEnter is applied before enter-active classes apply that var alone should suffice 🤔
| const doublePanelItem = | ||
| doublePanel && panelOpenForItemId && sections | ||
| ? findFirstLevelItem(sections, panelOpenForItemId) | ||
| : undefined; | ||
| const doublePanelChildren = doublePanelItem?.children; | ||
| const isDoublePanelOpen = Boolean(doublePanelChildren?.length); | ||
|
|
||
| if (isDoublePanelOpen && doublePanelItem && doublePanelChildren) { | ||
| lastDoublePanelContentRef.current = {label: doublePanelItem.label, children: doublePanelChildren}; | ||
| } | ||
| const doublePanelContent = lastDoublePanelContentRef.current; |
There was a problem hiding this comment.
I think having a simple code justifies having extra renders, few extra renders won't impact performance. This code along with previousSelection state var is a smell to me, something looks to be off. I think not having a panelOpen state var is the problem. We should have a panelOpenItemId state var and a panelOpen state var. That way we can toggle panel with panelOpen while just keeping the panelOpenItemId set (thus not needing the ref).
| onSelectedItemIdChange?: (id: string | null) => void; | ||
| }; | ||
|
|
||
| const SidenavBarContext = React.createContext<SidenavBarContextValue>({ |
There was a problem hiding this comment.
Mmm not nice to have, I would simplify this so next person that needs to makes changes can understand it in less time (or even an AI)
| </> | ||
| )} | ||
| </div> | ||
| {footerSlot && fixedFooter && ( |
There was a problem hiding this comment.
When the content goes under fixed footer, a divider should be shown, same logic as drawer footer
|
|
||
| // Panel (Dialog and Double Panel) ----------------------------------------------- | ||
|
|
||
| export const dialogPanel = style({ |
There was a problem hiding this comment.
Items content and title are not aligned in dialog panel
Current:
Expected:
| const SECTION_TITLE_INSET = 24; | ||
| // Vertical space between two first-level entries of the body (section to section, section to | ||
| // stand-alone item, or stand-alone item to stand-alone item). Items inside a section stay adjacent. | ||
| const FIRST_LEVEL_GAP = 8; |
There was a problem hiding this comment.
| indicator: skinVars.colors.controlActivatedBrand, | ||
| hover: skinVars.colors.backgroundContainerBrandHover, | ||
| pressed: skinVars.colors.backgroundContainerBrandPressed, | ||
| selected: skinVars.colors.backgroundSelectedBrand, |
There was a problem hiding this comment.
This is not working in cyber
| isDoublePanelOpen && | ||
| doublePanelItem && | ||
| doublePanelChildren && |
There was a problem hiding this comment.
isDoublePanelOpen already implies the other two conditions, right?
| const closePanelForSelection = React.useCallback( | ||
| (selectionId: string | null) => { | ||
| if (doublePanel) { | ||
| setDismissedSelection(selectionId); | ||
| } | ||
| setPanelOpenForItemId(null); | ||
| }, | ||
| [doublePanel] | ||
| ); |
There was a problem hiding this comment.
Logic attached to this function is dead, we are not reopening the panel on navigations from outside in controlled mode, and I think that's the right behaviour. You can remove all related code and probably this prop from the context altogether and just use setPanelOpenForItemId from sidenav items.
| // The column closes as soon as the double panel mode goes off. Only the change of the mode closes it: | ||
| // the collapsed rail opens its dialog panel through this same state with the mode off. | ||
| const [previousDoublePanel, setPreviousDoublePanel] = React.useState(doublePanel); | ||
| if (doublePanel !== previousDoublePanel) { | ||
| setPreviousDoublePanel(doublePanel); | ||
| if (!doublePanel) { | ||
| setPanelOpenForItemId(null); | ||
| } | ||
| } | ||
|
|
||
| // A change of the entries invalidates the open column, whose parent item may not exist anymore. | ||
| const [previousSectionsLength, setPreviousSectionsLength] = React.useState(sections?.length ?? 0); | ||
| if ((sections?.length ?? 0) !== previousSectionsLength) { | ||
| setPreviousSectionsLength(sections?.length ?? 0); | ||
| setPanelOpenForItemId(null); | ||
| } |
There was a problem hiding this comment.
I would remove these corner cases handling... Perhaps the parent can use key prop to force rerender if this actually happens... wdyt?
| /** Variant of the top bar. The panel always renders in the default variant. */ | ||
| variant: NonDeprecatedVariant; | ||
| /** Logo of the top bar. */ | ||
| logo?: SidenavLogo; |
There was a problem hiding this comment.
I can see an inconsistency between mainnavigationBar and sidenav logo management
Using undefined as value in logo props in boths components
Example
| display: 'flex', | ||
| flexDirection: 'column', | ||
| alignItems: 'center', | ||
| gap: 32, |
There was a problem hiding this comment.
I dont know if this is the space.. but this space should be 24px. The same as 24px in bottom padding
An example to understand this. enable and disable Header Slot to see the difference with the red line
| * ] | ||
| * } | ||
| */ | ||
| type SidenavSection = { |
There was a problem hiding this comment.
there are a weird spacing between divider and icon when the sidenav is collapse and section titles have values
disabling section titles in this example you fix this problem. So, when the sidenav is collapse, section title in the sidenav should be hidden
| fixedFooter: boolean; | ||
| boxed: boolean; | ||
| divider: boolean; | ||
| sectionDividerTop: boolean; |
There was a problem hiding this comment.
missing a control to show/hide section titles
| */ | ||
| type SidenavCollapseActionRenderProps = { | ||
| /** Current collapsed state, which gives the direction of the action. */ | ||
| collapsed: boolean; |
There was a problem hiding this comment.
when you use :
collapsible={true}
collapsed={false}
The icon appear but is not working (logic)... but, why not hide the icon when collapsed is false? 🤔
| */ | ||
| type SidenavCollapseActionRenderProps = { | ||
| /** Current collapsed state, which gives the direction of the action. */ | ||
| collapsed: boolean; |
There was a problem hiding this comment.
could be defaultCollapsed a better naming for this?
| /** Display text (truncated if too long). */ | ||
| label: string; | ||
| /** Icon component or element. Required for top-level items in collapsed sidenav. */ | ||
| asset?: ((props: IconProps) => JSX.Element) | React.ReactElement; |
There was a problem hiding this comment.
Asset is optional.. but when you have collapsable in true, asset is required to avoid "ghost" items in the collapsed sidenav.
Is this something to aware in storybook? or any idea to avoid the wrong usage?
| }, | ||
| }); | ||
|
|
||
| export const logoContainer = style([ |
There was a problem hiding this comment.
Logo container is not adapting to height, should be? IMO yes. If the height of the logo is 72, this container height should be 72, fitting the height
There was a problem hiding this comment.
ellipsis is an accessibility problem
| /** Custom content below logo/collapse in header. */ | ||
| headerSlot?: React.ReactNode; | ||
| /** Custom background colors for header (opaque), body (any), and footer (opaque) regions. */ | ||
| background?: SidenavBarBackgroundColors; |
There was a problem hiding this comment.
this background is not working, right?
Add screenshot test for this case
There was a problem hiding this comment.
focus ring is overflowing
There was a problem hiding this comment.
focus ring is not activating the hover state background
Summary - AI;DR
This PR adds the
SidenavBarcomponent and the desktop, mobile, and double-panel behaviours from the Figma spec. Read the sections below in order. They tell you where to look and what still blocks the merge.Bulky PR
I'm considering opening a PR to simply import new icons as well as the new skins (along with the new tokens we need).
This way, the PR would look less cluttered and bulky. WDYT? Let me know.
Specs
The markdown spec and the token branch live on the
aweell-generate-figma-specsbranch ofmistica-design. That branch is not final. See the blockers below.Review guide
1. Shared mobile module (start here)
The mobile sidenav must look and behave like the mobile nav bar. So this PR extracts the common parts of
navigation-barinto a shared module. Both components now consume it.src/navigation-shared.tsx,src/navigation-shared.css.ts,src/navigation-types.ts,src/mobile-navigation-menu.tsx— the new shared code.src/navigation-bar.tsxandsrc/navigation-bar.css.ts— reduced, because they delegate to the shared module. Check that the nav bar still behaves the same.2. Screaming architecture for
src/sidenav-bar/Everything for this component lives under one directory: implementation, styles, context, mobile, motion, keyboard, tests, stories, and snippets. The intent is faster work for a person and for an AI agent, because one directory holds the full feature. An agent reads one tree instead of searching the repo, so it spends fewer tokens.
jest.unit.config.jsand.eslintrc.yml.3. Blockers — you must not merge before we fix these
// TODO WIPsrc/text-tokens.tsx— the es/de/pt strings are drafts. They need official localization sign-off. Only en is final.src/sidenav-bar/sidenav-bar.css.ts:89— the media variant hover and pressed tokens are missing. They come from theaweell-generate-figma-specsbranch ofmistica-designthat is not final yet.src/sidenav-bar/sidenav-motion.tsx:17— an open question about whether to promote the motion hook tosrc/hooks.4. Repo harness for specs and AI (support changes, not the feature)
This PR also improves how we work with specs and AI agents in this repo.
agents/skills/plus the tool-specific symlinks — theread-component-specsandrun-in-browserskills.scripts/browse-story.jsandscripts/open-story.js— drive a story in the CI Chromium Useful when it comes to measure things or check if we're complying spacing-mesasures etc, trying out things etc!5. IntelliJ IDEA project files
This PR adds IntelliJ project files, the same way we already ship Visual Studio Code files. They make the repo easy to open in IntelliJ.
Ref: 1592
Pre-merge checklist
You must not merge before you address every item below.
mistica-designwith all the missing tokens.// TODO WIPmarkers in the code.