fix(Navigation): keep the badge in the item's line and truncate the label - #3070
fix(Navigation): keep the badge in the item's line and truncate the label#3070Lisa18289 wants to merge 6 commits into
Conversation
Coverage Report for ./packages/components/
File CoverageNo changed files found. |
🚀 Preview DeploymentPreview environments are ready:
Images:
|
❌ Visual Regression Tests FailedAt least one shard did not pass. If snapshots differ from the committed baselines, download the visual-diffs-* artifacts from this run to inspect the actual/diff images (a shard that failed before comparison, e.g. during install or browser setup, leaves none). If the differences are intentional, update the baselines by adding the |
349bafa to
c686e27
Compare
…abel #2903 let the item wrap so a badge moved to the next line. In a narrow navigation that broke the label off the icon instead, so keep one line and truncate the label. A label passed as raw text is an anonymous flex item that no rule can reach, so the link's props context wraps it in an element. Remote delivers text as a renderer element rather than a string — without that branch only Local truncated. `min-width: 0` on the navigation is what makes truncation happen at all in a flexible container: a `1fr` grid track takes the content's min-content width as its minimum, so the navigation grew past its column instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Lisa18289 <84317589+Lisa18289@users.noreply.github.com>
c686e27 to
8a00734
Compare
| /** Text a link receives as a raw string, or as a remote text node. */ | ||
| const isText = (child: ReactNode): boolean => | ||
| typeof child === "string" || | ||
| typeof child === "number" || | ||
| (isValidElement(child) && isRemoteTextRenderProps(child.props)); | ||
|
|
||
| /** | ||
| * A bare text node is an anonymous flex item that no rule can reach, so give | ||
| * the label an element the item can truncate. | ||
| */ | ||
| const wrapTextInLabel = (children: ReactNode): ReactNode => | ||
| Children.map(children, (child) => | ||
| isText(child) ? <span>{child}</span> : child, | ||
| ); |
There was a problem hiding this comment.
isText duplicates a predicate two other open PRs also carry, and one of them already puts it in the right place.
- fix(Button): treat a string label like a Text child #3053 adds
isTextNode+ an exportedcontainsTextChildtosrc/lib/react/remote.ts— same three branches, plus fragment recursion. (It missesnumber; I asked for that there so this PR can drop its helper without losing a case.) - fix(Option): infer textValue from mixed children and keep value stable #3076 adds
extractTextFromChildrento the same file, with the same classification inside its walk.
containsTextChild applied per child answers exactly the question here — containsTextChild("Galactic") is true, containsTextChild(<Badge/>) is false — so both local helpers collapse into one call:
| /** Text a link receives as a raw string, or as a remote text node. */ | |
| const isText = (child: ReactNode): boolean => | |
| typeof child === "string" || | |
| typeof child === "number" || | |
| (isValidElement(child) && isRemoteTextRenderProps(child.props)); | |
| /** | |
| * A bare text node is an anonymous flex item that no rule can reach, so give | |
| * the label an element the item can truncate. | |
| */ | |
| const wrapTextInLabel = (children: ReactNode): ReactNode => | |
| Children.map(children, (child) => | |
| isText(child) ? <span>{child}</span> : child, | |
| ); | |
| /** | |
| * A bare text node is an anonymous flex item that no rule can reach, so give | |
| * the label an element the item can truncate. | |
| */ | |
| const wrapTextInLabel = (children: ReactNode): ReactNode => | |
| Children.map(children, (child) => | |
| containsTextChild(child) ? <span>{child}</span> : child, | |
| ); |
Two imports follow: line 13 becomes import { containsTextChild } from "@/lib/react/remote";, and isValidElement on line 2 falls away (import { Children } from "react";).
Worth having beyond the deduplication: containsTextChild looks through fragments, so a label passed as <>{"Text"}</> gets wrapped and truncated too. isText returns false for that today.
No rendered output changes for the scenarios in this PR, so the committed baselines stay valid.
Order: this waits on #3053. If that one stalls, merging here as-is is fine — the duplication is then temporary and whichever of #3053/#3076 lands second cleans it up (those two conflict in remote.ts and need a rebase against each other anyway).
There was a problem hiding this comment.
Applied in 08926fb, after merging #3053's main in.
I first went down a wrong path here and want to record why, because it nearly cost this PR the change. I read containsTextChild as an aggregate over a whole subtree and concluded it could not answer a per-child question, so I exported isTextNode instead. That was wrong on both counts — and isTextNode is the one symbol #3076 removes, so it would have left this PR depending on something that disappears when yours lands. containsTextChild stays exported there.
Applied per child it is exactly right, verified:
| child | result |
|---|---|
"Galactic" |
true |
{42} |
true |
<Badge /> |
false |
<Badge>New</Badge> |
false |
<>{"Text"}</> |
true |
The fourth line is the one I had doubted — text inside an element stays that element's, so it is not wrapped. And the fifth is the improvement you mentioned; the local helper returned false for it.
One note for when #3076 lands: your containsTextChild is extractTextFromChildren(children) !== undefined, which trims, so a whitespace-only child ({" "} between two elements) stops counting as text and no longer gets a <span>. That reads like the better behaviour to me — flagging it only because it is a silent change to this call site.
test:compile and lint clean, unit 253/253, the Navigation visual scenarios pass against the committed baselines unchanged.
#3053 landed the same predicate in `remote.ts`. Applied per child it answers exactly this question, and it looks through fragments, so `<>{"Text"}</>` is wrapped and truncated too — the local helper returned false for that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#2903 let a navigation item wrap so its badge could move to the next line. In a narrow navigation that wraps the label off its icon instead, so keep everything on one line and truncate the label.
A label passed as raw text is an anonymous flex item that no rule can reach — the link's props context wraps it in an element. Remote delivers text as a renderer element rather than a string, so that case is matched too; without it only
Localtruncated.min-width: 0on the navigation is what makes truncation happen at all inside a flexible container: a1frgrid track takes the content's min-content width as its minimum, so the navigation grew past its column instead of shrinking.The
narrow containervisual scenario now uses labels that actually overflow — before, every label fit and the scenario could not have shown the difference — with and without a leading icon, plus aTruncatechild (what mStudio passes).Needs the
update-screenshotslabel for the Linux baselines; only the-darwinones can be written locally.