feat(badge): unify badge interface to support array values in DescriptionCard and DataTable to render multiple badges#278
Conversation
commit: |
|
/review |
…interface - DescriptionCard badge field now accepts array values, rendering multiple badges - Extract shared BadgeVariant type and BadgeOptions interface into badge-utils.ts - DataTable's BadgeCellOptions now extends shared BadgeOptions - Add badgeLabelMap and defaultBadgeVariant to DescriptionCard FieldMeta - Deprecate BadgeVariantType in favor of shared BadgeVariant - Add resolveBadgeVariant/resolveBadgeLabel shared utilities - Add example of array badge usage in vite-app
- Add maxVisible option to FieldMeta for badge fields - Overflow badges shown in a popover on click with full Badge UI - +N trigger is a simple text element with hover highlight (no dot) - Add popover interaction test
- Extend badge accessor type to accept string[] in addition to primitives - renderBadge handles arrays with multiple Badge rendering - Add maxVisible + Popover overflow (same pattern as DescriptionCard) - Update type test: badge accessor now allows arrays - Add rendering tests for multi-badge and maxVisible in DataTable
- Merge badge-utils.ts into badge-list.tsx as single source of truth - Remove default render from inferColumns() to fix type renderer priority - Add renderText fallback for boolean/Date/object values - Add openOnHover to BadgeList overflow popover - Add tags badge column to DataTable demo (multiple badges + maxVisible) - Convert status column from custom render to type: badge - Add comprehensive tests for BadgeList, renderText fallback, and field-helpers
- renderBadge now returns placeholder for empty arrays (consistent with DescriptionCard) - Align default badge variant to 'outline-neutral' across DataTable and DescriptionCard - Remove redundant BadgeVariant override in DescriptionCard field-renderers - Add changeset
c967c63 to
412c160
Compare
… test - renderBadge now filters array items and returns placeholder when all values are null or empty strings - Add tests for [null, null] and ["", ""] badge column cases - Change field-renderers test to use user.hover instead of user.click to match the openOnHover popover behavior
| */ | ||
| function BadgeFieldRenderer({ field }: { field: ResolvedField }) { | ||
| if (isEmpty(field.value)) { | ||
| const values = Array.isArray(field.value) |
There was a problem hiding this comment.
Can we make this into an if/else function instead? Nested ternaries are a bit hard to read/understand
There was a problem hiding this comment.
Agreed. I guess nested ternaries should be prohibied by linting.
There was a problem hiding this comment.
I would like to update oxlint rules to prevent nested ternaries, so let me work on it in the next PR.
| resolveLabel: resolveLabelProp, | ||
| badgeClassName, | ||
| }: BadgeListProps): React.ReactNode { | ||
| const values = Array.isArray(value) ? value : [value]; |
There was a problem hiding this comment.
From an API design perspective, wouldn't it make more sense to force value to always be an array even if there's only one? That way we don't have to check the variable type and consistently enforce it as an array through TS
There was a problem hiding this comment.
Good point to raise.
The reason I went with T | T[] is to optimize for caller ergonomics — most usages will pass a single value, and forcing callers to wrap it as an array adds repeated boilerplate at every call site. Internal type consistency is preserved by normalizing on the first line, so the rest of the component still operates on T[] with full type safety.
This also matches how React's own APIs behave — children, style, and similar props all accept T | T[] and normalize internally, which I think sets a reasonable precedent for forgiving input shapes in shared components.
Given AppShell is consumed across many surfaces — including by AI coding agents that often forget the [x] wrap and incur an extra type-error-then-fix round trip — I'd lean on this pattern to minimize friction at call sites.
There was a problem hiding this comment.
Yeah fair enough, as long as coding agents consistently follow this pattern I don't see an issue :)
| if (isEmpty(value)) return PLACEHOLDER; | ||
| if (typeof value === "boolean") return value ? "✓" : "✗"; | ||
| if (value instanceof Date) return value.toLocaleDateString(); | ||
| if (typeof value === "object") return JSON.stringify(value); |
There was a problem hiding this comment.
Is this intentional? The value can look pretty strange with this
I also think it might more sense to convert this to a Switch case command for readability
There was a problem hiding this comment.
Here is the mixture of multiple types of comparison using typeof and instanceof, so can we really make everything in one switch?
There was a problem hiding this comment.
I think that'll be fine, because switch evaluates on a boolean, this can be like:
function renderText(value: unknown): ReactNode {
switch (true) {
case isEmpty(value):
return PLACEHOLDER;
case typeof value === "boolean":
return value ? "✓" : "✗";
case value instanceof Date:
return value.toLocaleDateString();
case typeof value === "object":
return JSON.stringify(value);
default:
return String(value);
}
}
There was a problem hiding this comment.
I'm not super fussed though, it's only 4 conditions so it's not too bad right now, but if we want to add more conditions it might be useful.
More concerned about stringifying objects, maybe it should throw an error instead?
There was a problem hiding this comment.
siwtch (true) looks stylish, but I remember it disables type narrowing, doesn't it? I don't like that if so.
More concerned about stringifying objects, maybe it should throw an error instead?
I cannot ensure if throwing errors there does not break any of the current projects that use AppShell, so best not touch that for backward behaviour compatibility. Showing data to users is not harmful if that's just raw JSON.
There was a problem hiding this comment.
siwtch (true) looks stylish, but I remember it disables type narrowing, doesn't it? I don't like that if so.
Ah, I probably didn't know we can do it now? Then, let's go with switch one!
There was a problem hiding this comment.
Ah, I probably didn't know we can do it now? Then, let's go with switch one!
Yeah since TS version 5.4 it's possible, but like I said, there's only 4 conditions so I'm okay if we don't do it haha.
I cannot ensure if throwing errors there does not break any of the current projects that use AppShell, so best not touch that for backward behaviour compatibility. Showing data to users is not harmful if that's just raw JSON.
Ah right, yeah I guess in that case it's fine, we should probably start enforcing it in the future though.
There was a problem hiding this comment.
Refactored renderText to use switch(true) in 92e2900.
| options?.badgeVariantMap?.[key] ?? options?.defaultBadgeVariant ?? "neutral"; | ||
| const label = options?.badgeLabelMap?.[key] ?? key; | ||
| return <Badge variant={variant}>{label}</Badge>; | ||
| const items = Array.isArray(value) ? value : value != null ? [value] : []; |
There was a problem hiding this comment.
Same as below, I think we should enforce this to remain an array so we don't constantly transform/re-transform it
There was a problem hiding this comment.
Maybe #278 (comment) will answer this too?
| case isEmpty(value): | ||
| return PLACEHOLDER; | ||
| case typeof value === "boolean": | ||
| return value ? "✓" : "✗"; |
There was a problem hiding this comment.
ふと思ったんですが、ここは✓✕より○✕の方が日本っぽくないですか? 😄
There was a problem hiding this comment.
たしかにそうですね。しかし、このbooleanのfallback rendererって使ってる人いない可能性もあるので、撤廃してもいい可能性あります。
Summary
In typical AppShell applications, DataTable is used on list pages and DescriptionCard on detail pages — users navigate from a table row to its detail view. To provide a consistent badge experience across this common pattern, this PR unifies badge rendering into a shared
BadgeListcomponent that both DataTable and DescriptionCard use, with support for array values and overflow handling (maxVisible+ hover popover).Usage
Screenshots
DescriptionCardwith multiple badges:DataTablewith multiple badges:Changes
BadgeList component (
badge-list.tsx)badge-utils.tsintobadge-list.tsxas single source of truthBadgeListcomponent renders one or more badges with overflow popoveropenOnHover)BadgeVariant,BadgeOptions,BadgeListProps,BadgeList,resolveBadgeVariant,resolveBadgeLabelArray badge support
string[])string[]from accessorUnified badge interface
BadgeOptionsinterface used by both DataTable and DescriptionCardresolveBadgeVariant()/resolveBadgeLabel()shared utilities"outline-neutral"across both DataTable and DescriptionCard (previously DataTable used"neutral"and DescriptionCard used"outline-neutral")BadgeCellOptionsextendsBadgeOptionswithmaxVisibleBadgeVariantTypeas a deprecated alias for backward compatibilityrenderBadgenow correctly shows—placeholder for empty arrays (consistent with DescriptionCard)inferColumns() fix
renderfrominfer()return value sotyperenderers take prioritycolumn({ ...infer("field"), type: "badge" })would use the genericformatValuerender instead of the badge rendererrenderTextfallback for boolean (✓/✗), Date (toLocaleDateString), and object (JSON.stringify) to avoid regressionstypeorrendernow display—for null/empty values (aligns with typed-column behavior)DataTable demo
tagsfield with multiple badges andmaxVisible: 2to product tablestatuscolumn from customrenderto declarativetype: "badge"+typeOptionsTests
badge-list.test.tsx: 18 tests covering utilities and component behaviordata-table.test.tsx: Added tests for renderText fallback (boolean, Date, object), array badge, and empty array placeholderfield-helpers.test.ts: Updated expectations for removedrenderproperty