Repository navigation
fix(core): ReactNode slots use isRenderable for 'renders nothing' - #9160
Conversation
Badge's label check (from #8236) becomes isRenderable, with the same result. The same hand-rolled check on other ReactNode slots now uses it too, so true, false and '' count as empty the way React renders them: DropdownMenuGroup's title, and the Tokenizer sheet's emptySearchText (custom element and string title). Each has a test for '', false, true and null, plus 0, and each test fails without its change. Co-authored-by: vjeux <vjeux@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsBadge (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: Accessibility did not run on this pull request; add Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The DropdownMenuGroup title={true} fix is pinned by a test that fails without it, and the Badge consolidation keeps its behaviour. The sheet Tokenizer change, though, replaces emptySearchText values the caller passed ('', false, true) with the default "No results found" message, while the prop documents '' as the way to render nothing and the popover renders nothing for all three; please drop that change or keep only null and undefined falling through to the default. The changeset also reads as library-wide (lines 5 and 11) although three slots change, so please scope it to what this PR changes.
[Automated review]
| [fix] Components treat a ReactNode slot that renders nothing (`null`, `undefined`, `true`, `false`, `''`) the same way React does, through one shared check: | ||
|
|
||
| - A `DropdownMenuGroup` with a `title` of `true` no longer renders an empty heading that names the group. | ||
| - A sheet-presented `Tokenizer` with an `emptySearchText` of `false`, `true` or `''` shows the default "No results found" state. Before, it showed an empty box or an empty title. | ||
| - `Badge` uses the same shared check for its label (thanks @vjeux, #8236). Its behavior is unchanged. | ||
|
|
||
| `0` still counts as content everywhere, since React renders it. |
There was a problem hiding this comment.
This release note reads as library-wide, but the PR changes three slots. The public Tokenizer popover still renders an empty results panel for emptySearchText={false} (BaseTypeahead resolves it with ??). TextArea's startIcon && … still hides 0, as the PR description notes. The sheet Tokenizer isn't reachable through public TokenizerProps yet. Suggested wording:
[fix] `DropdownMenuGroup`'s `title`, `Badge`'s `label` and the not-yet-public sheet presentation of `Tokenizer` now use the shared `isRenderable` check, so `null`, `undefined`, `true`, `false` and `''` count as rendering nothing; `0` still counts as content.
…anges Review follow-up: the note read as library-wide. It now names Badge's label, DropdownMenuGroup's title and the not-yet-public sheet Tokenizer, and says other slots (the popover Tokenizer's emptySearchText included) are unchanged.
There was a problem hiding this comment.
Changes requested. The rescoped changeset now matches what changes, and the DropdownMenuGroup title={true} fix and the Badge consolidation are good as they stand. The sheet Tokenizer hunk still turns emptySearchText values the caller passed ('', false, true) into the default "No results found" message, shown and announced, while the prop documents '' as the way to render nothing and the public popover renders nothing for all three. Please conform the sheet so only null and undefined fall through to the default (the inline suggestion does this and keeps the rest of the sheet suite passing), or drop that hunk along with its test and the sheet clause in the changeset.
[Automated review]
| const hasCustomEmptyElement = | ||
| typeof emptySearchText !== 'string' && emptySearchText != null; | ||
| isRenderable(emptySearchText) && typeof emptySearchText !== 'string'; |
There was a problem hiding this comment.
Only null and undefined are "not given" for this prop ("Pass an empty string to render nothing", Tokenizer.tsx:256-257), and the popover renders '', false and true as nothing. Here those three fall through to the default "No results found" state, shown and announced. This predicate makes the sheet match the popover for every value:
const hasCustomEmptyElement =
emptySearchText != null &&
(typeof emptySearchText !== 'string' || emptySearchText === '');The new test at line 647 and the changeset's sheet clause would change with it.
Review follow-up: the prop documents '' as the way to render nothing, and the popover renders '', false and true as nothing, so turning them into the default empty state contradicted the prop. The sheet keeps its current check; the PR body lists it among the sites left, and the changeset names only Badge's label and DropdownMenuGroup's title.
The previous commit restored it from a newer main; it now matches the base this branch forked from, so the PR no longer touches it.
…he nullish-JSX-guard baseline Badge's label and DropdownMenuGroup's title now use isRenderable, so their entries leave no-nullish-jsx-guard.baseline.json and strict lint stays clean.
There was a problem hiding this comment.
This looks ready to approve. Moving DropdownMenuGroup to the shared isRenderable check stops title={true} from rendering an empty heading that names the group or overrides a caller's aria-labelledby, and the new test fails without the fix; Badge keeps identical behaviour. The earlier requests about the sheet Tokenizer change and the changeset's scope are resolved at this head.
[Automated review]
User impact
DropdownMenuGroupwithtitle={true}rendered an empty heading, and the group was named by it.isRenderablecheck, sonull,undefined,true,falseand''count as empty, the way React renders them.0still counts as content, since React renders "0".Badgebehaves exactly as before. Its hand-rolled check (from fix(Badge): a badge grows with its text #8236) was already the same asisRenderable.Spec
component:DropdownMenuGroup(current) FR1/FR2 and AV1: a titled group is named by its visible heading, and "the title may be any React node". A title that renders nothing leaves no visible heading, so the group is untitled (FR2).Changes
Badge.tsx:hasLabel = isRenderable(label). Same result as before.DropdownMenuGroup.tsx:hasTitle = isRenderable(title). It wastitle != null && title !== '' && title !== false, sotruegot through.isRenderable: unchanged. No new helpers or exports.isRenderableversus the old checksisRenderablenull,undefinedfalsetrue''0[],<></>isRenderableis shallow on purpose: arrays and fragments count without looking inside, as before at every site.Sites deliberately left
Grouped by why, so the remaining sites can be migrated with these rules. The search covered
packages/coreandpackages/lab.1. String or data checks: not ReactNode slots, leave as they are
These test a
stringprop or a data value, andisRenderableis for ReactNode slots only.Avatar/Avatar.tsx:495(accessibleName: string) and:650(trimmed tooltip text);Icon/Icon.tsx:284(label: string);Timestamp/TimestampHoverCard.tsx:227(line.label: string);Chat/ChatComposer.tsx:486(status.message: string);Comments/CommentParts.tsx:618(trimmed avatar URL) and labComments/i18n.ts:35(description: string).SideNav/SideNavItem.tsx:680andDropdownMenu/DropdownMenu.tsx:559;Layer/useTouchTrigger.ts:103andhooks/scrollKeyboardDelegation.ts:114.Layer/layerInset.ts:40andgestures/nativeScroll.ts:170.utils/plainDate.ts:454,CommandPalette/CommandPalette.tsx:467,gestures/haptics.ts:120,Table/columnUtils.ts:335,motion/animatedLayout.ts:1609,Markdown/plugins/headingLinks.ts:235;Markdown/remark.tsandMarkdown/plugins/protocol.ts(mdast fields).2.
falseis a documented opt-out: keep the explicit checkAppShell/AppShell.tsx:548/558:mobileNav={false}turns the mobile nav off, and the check also tells a config object from an element.Table/BaseTable.tsx:590:emptyState={false}turns the empty state off, whilenull/undefinedfall back to the default.Tokenizer/TokenizerBottomSheet.tsx:668/683(emptySearchText): the prop says "pass an empty string to render nothing", and the popover renders'',falseandtrueas nothing, so anisRenderabledefault here would contradict the prop.3.
isRenderablewould visibly change0: needs a decision firstTextArea/TextArea.tsx:625/676:startIcon && …andBoolean(startIcon).MobilePowerSearch/MobilePowerSearch.tsx:1915:endContent || ….0renders "0" in React, soisRenderablewould show a lane that the truthiness check hides today.4. Governed by its record
Chat/ChatMessageList.tsx:256(hasChildren):ChatMessageListFR4 says the empty state shows only for nullish,falseor an empty top-level array. So''ortruechildren keep today's behavior until that record changes.5. A different shape:
??defaults on ReactNode slotsfalseand''pass through??as content, so the default never shows.Typeahead/BaseTypeahead.tsx:490(emptySearchText, used by the popoverTokenizer),Selector/Selector.tsx:1011/1013(emptyText,emptySearchText),Chat/ChatComposer.tsx:573/584(input,sendButton).Lint baseline
Badge/Badge.tsxandDropdownMenu/DropdownMenuGroup.tsxfromno-nullish-jsx-guard.baseline.json, since both now useisRenderable.Evidence
'',false,trueandnulleach, plus0:[]and an empty fragment;check:changesetspass.Credit: @vjeux for Badge's check (#8236).