fix(MainNavigationBar): allow right slot to expand#1546
fix(MainNavigationBar): allow right slot to expand#1546AlexandraGallipoliRodrigues wants to merge 22 commits into
Conversation
|
Size stats
|
|
Deploy preview for mistica-web ready!
Deployed with vercel-action |
|
Accessibility report ℹ️ You can run this locally by executing |
There was a problem hiding this comment.
Pull request overview
This PR fixes layout limitations in MainNavigationBar/NavigationBar right content so the right slot can properly expand when the provided right element is intended to be full-width, and introduces a new InlineItem API to allow individual inline children to opt into flex-grow behavior.
Changes:
- Wrap
rightcontent inNavigationBarContentContainerwhenexpandRightContentis enabled and the right element hasfullWidth, plus add supporting CSS to allow correct shrinking/expansion. - Add
InlineItemwithgrowsupport so specificInlinechildren can expand, and switchInlinerendering to inspect children for grow items. - Re-export
InlineItemfrom the library entrypoint.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/navigation-bar.tsx | Conditionally wraps right slot content to allow full-width expansion when requested. |
| src/navigation-bar.css.ts | Tweaks expanded right-slot sizing and adds a wrapper style for full-width content. |
| src/inline.tsx | Adds InlineItem API and updates child rendering to support grow items. |
| src/inline.css.ts | Adds flex container and grow-item styles needed for the new InlineItem behavior. |
| src/index.tsx | Exposes InlineItem from the public package API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
I dont feel really easy in terms of devExp to have another component called InlineItem to manage this kind of things
Is not there any other solution? maybe manage this property in the <Inline> component?
wdyt @atabel ?
There was a problem hiding this comment.
Good point, thanks! I’ve removed the public InlineItem component and moved the behavior into Inline via a growItems prop
…OBVIVO-3456-fix-MainNavigationBar-allow-right-slot-to-expand
|
Size stats
|
| hasGrowItem && styles.flexLayout, | ||
| isStringSpace ? (wrap ? styles.stringSpaceWithWrap : styles.stringSpace) : styles.marginInline |
…OBVIVO-3456-fix-MainNavigationBar-allow-right-slot-to-expand
…ow-right-slot-to-expand
|
Screenshot tests report ✔️ All passing |
…ow-right-slot-to-expand
| ]); | ||
|
|
||
| export const navigationBarContentRightExpandedContent = style({ | ||
| width: '100%', |
There was a problem hiding this comment.
What about an expand prop on the Inline element? It would set its width to 100% whenever it is set. This way we wouldn't have to modify navigation bar I think. This prop could accept what you currently have in flexGrow prop and wouldn't need fullWidth to be set.
| const childrenArray = React.Children.toArray(children).filter((child) => !!child || child === 0); | ||
|
|
||
| const hasExpandItem = childrenArray.some((_, index) => shouldExpandItem(expand, index)); | ||
| const shouldExpand = expand !== undefined; | ||
|
|
||
| return ( | ||
| <div | ||
| className={classnames( | ||
| className, | ||
| styles.inline, | ||
| wrap ? styles.wrap : fullWidth ? styles.fullWidth : styles.noFullWidth, | ||
| isStringSpace ? (wrap ? styles.stringSpaceWithWrap : styles.stringSpace) : styles.marginInline | ||
| wrap ? styles.wrap : fullWidth || shouldExpand ? styles.fullWidth : styles.noFullWidth, | ||
| isStringSpace | ||
| ? wrap | ||
| ? styles.stringSpaceWithWrap | ||
| : styles.stringSpace | ||
| : styles.marginInline, | ||
| shouldExpand && styles.expand | ||
| )} |
| dataAttributes?: DataAttributes; | ||
| wrap?: boolean; | ||
| /** | ||
| * Index or indexes of the children that should grow to fill the available space. |
| const {isDesktopOrBigger} = useScreenSize(); | ||
|
|
||
| const right = expandedRightSlot ? ( | ||
| <Inline fullWidth space={16} alignItems="center" expand={0}> |
There was a problem hiding this comment.
I think we can skip fullWidth prop here, right?
|
|
||
| export const expandItem = style({ | ||
| flex: 1, | ||
| minWidth: 0, |
There was a problem hiding this comment.
minWidth: 0 was intentional here. In a flex layout, flex items have min-width: auto by default, so an expanded item can refuse to shrink below its content size and push adjacent items out of the available space. Setting minWidth: 0 allows the expanded item to shrink properly and lets inner components handle truncation/overflow... But i haven't been able to prove this case in a playroom, should I delete the minWidth then?
There was a problem hiding this comment.
Hmm I don't know if that behaviour is desirable, perhaps we could live without it for the moment...
https://developer.mozilla.org/en-US/play?id=pEK8gpiRaKCWYKNxPpXPkgOoAEQy8NkIAdjrAPjKL0gP%2BAzWTL%2FLoG9rA2WXLAksvUN8leTXOzoc46Dg
| {childrenArray.map((child, index) => { | ||
| return ( | ||
| <div | ||
| key={getChildKey(child, index)} |
| }: Props): JSX.Element => { | ||
| const {platformOverrides} = useTheme(); | ||
| const isStringSpace = typeof space === 'string'; | ||
| const childrenArray = React.Children.toArray(children).filter((child) => !!child || child === 0); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
added !wrap Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| * Indexes refer to entries in `React.Children.toArray(children)`, so empty nodes | ||
| * (`null`, `undefined` and booleans) are ignored, but React elements still count | ||
| * even if they ultimately render no content. Expanded children must render content | ||
| * to produce a visible expanded item. |
| className, | ||
| styles.inline, | ||
| wrap ? styles.wrap : fullWidth ? styles.fullWidth : styles.noFullWidth, | ||
| isStringSpace ? (wrap ? styles.stringSpaceWithWrap : styles.stringSpace) : styles.marginInline | ||
| wrap ? styles.wrap : fullWidth || hasExpandItem ? styles.fullWidth : styles.noFullWidth, | ||
| isStringSpace | ||
| ? wrap | ||
| ? styles.stringSpaceWithWrap | ||
| : styles.stringSpace | ||
| : styles.marginInline, | ||
| hasExpandItem && !wrap && styles.expand | ||
| )} |
| const childrenArray = React.Children.toArray(children).filter((child) => child !== ''); | ||
|
|
||
| const hasExpandItem = childrenArray.some((_, index) => shouldExpandItem(expand, index)); | ||
| const shouldExpand = hasExpandItem && !wrap; |
| /** | ||
| * Index or indexes of the children that should grow to fill the available space. | ||
| * Indexes refer to entries in `React.Children.toArray(children)`, so empty nodes | ||
| * (`null`, `undefined` and booleans) are ignored, but React elements still count | ||
| * even if they ultimately render no content. | ||
| * | ||
| * This prop has no effect when `wrap` is enabled. | ||
| */ | ||
| expand?: number | ReadonlyArray<number>; |
| }); | ||
|
|
||
| export const expandItem = style({ | ||
| flex: 1, |


ticket: https://jira.tid.es/browse/WEB-2439
figma: https://www.figma.com/design/HA6UnnWC187IL1YujMYlYZ/Main-Navigation-Bar?node-id=2412-5583
Inline expandprop: allows you to specify which Inline child should occupy the remaining space.bug:

fix, local playroom:
