Repository navigation
fix(Badge): a badge grows with its text - #8236
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
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: The accessibility audit did not run on this pull request. It runs after merges to 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.
The Badge fix itself works: with 20px and 40px supporting text, badges now grow to 33.3px and 66.7px and keep their label inside, and default-size badges are pixel-identical across all seven first-party themes. Approval waits on an engineering owner's spec decision: this diff also carries its dependency stack, so an owner has to approve or reject the draft AST-094 requirements (FR1–FR23, DEC-1, DEC-2) and provide or identify the spec that governs the new typography.dynamicType theme option and <html data-astryx-text-size>, recording it by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner). Independently of that decision:
- Land the stack through its own pull request first (or retarget this one onto it), so this diff is the 168-line Badge commit.
- Update the branch from
main:pr-a11ycurrently stops atmain's new WheelPinchZoom contract, whose spec file this branch lacks, before it reaches the new Badge contract. - Remove, or land, the files that
packages/core/src/motion/MEASUREMENTS.mdand theuikitheaders point to (uikit/switch.ts,uikit/contextMenu.ts,decay.tsand others).
[Automated review]
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts is not in the tree at this head (nor is uikit/contextMenu.ts, line 254), so the switch rows marked done point at measured values nobody can find. Drop the reference or land the file with the table.
There was a problem hiding this comment.
The Badge fix works: with 20px and 40px supporting text, badges now grow to 33.3px and 66.7px and keep their label inside, default-size badges are unchanged in all eight Storybook themes on desktop and phone, and the new Chromium contract passes in CI. Approval waits on an engineering owner's spec decision, because this diff also carries its dependency stack: an owner has to approve or reject the draft AST-094 requirements (FR1–FR23, DEC-1, DEC-2) and provide or identify the spec that governs the new typography.dynamicType theme option and <html data-astryx-text-size>, including whether the reader's text size may take the phone base below the 16px that design:mobile-typography DR1 sets, recording it by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner). Independently of that decision:
- Land the stack through #8053 and #8230 first, or retarget this pull request onto #8230, so this diff is the 255-line Badge change.
- Correct or land what the stack points at but does not contain:
uikit/switch.ts,uikit/contextMenu.ts,decay.ts,rubberBand.ts,projection.ts,sources.test.ts, and thespec:AST-090that AST-094 FR7 amends.
[Automated review]
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
FR7 departs from “the draft spec:AST-090 FR1 … and FR10”, but no AST-090 record exists on main or at this head, and the Current-state impact repeats it (line 235). Cite the record that holds those exit and reduced-motion rules, or drop the amendment.
| * the rubber-band coefficient, the bounce-back spring, paging and the | ||
| * content-touch delay | ||
| * @position The scroll domain of the UIKit constants table (motion/uikit); | ||
| * read by decay.ts, rubberBand.ts, projection.ts and components |
There was a problem hiding this comment.
decay.ts, rubberBand.ts and projection.ts are not in the tree at this head, and neither is sources.test.ts (line 14; the enforcing test is uikit.test.ts). Point at what exists, or land those files with the table.
d831b32 to
94c6300
Compare
There was a problem hiding this comment.
The Badge fix works where it matters: with 1.25rem and 2.5rem supporting text the pill grows to 33.3px and 66.7px and keeps its label inside, default-size badges are pixel-identical, and the new Chromium contract passes in CI. Approval waits on an engineering owner's spec decision, because this diff also carries its dependency stack: an owner has to approve or reject the draft AST-094 requirements (FR1–FR23, DEC-1, DEC-2) and provide or identify the spec that governs the new typography.dynamicType theme option and <html data-astryx-text-size>, including whether the reader's text size may take the phone base below the 16px that design:mobile-typography DR1 sets, recording it by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner). Independently of that decision:
min-width: 1lhlets a badge escape a container narrower than one line (10.7px past a 56px cell at 2.5rem text), againstcomponent:BadgeFR3;min-width: min(1lh, 100%)keeps it inside.component:BadgeFR1 still records a fixed--spacing-5block size, so amend it with the one-line minimum and list the larger-text contract in its verification.- Land the stack through #8053 and #8230 first, or retarget onto #8230, so this diff is the 278-line Badge change, and correct the stack's references to files and to an AST-090 record that do not exist.
[Automated review]
| // label at least that wide. | ||
| maxWidth: '100%', | ||
| minWidth: 0, | ||
| minWidth: '1lh', |
There was a problem hiding this comment.
min-width wins over max-width: 100%, so a badge can no longer shrink below one line of its own text: at 2.5rem supporting text in a 56px cell every badge is 66.7px wide and ends 10.7px past the cell, where component:Badge FR3 keeps the root capped at its container. min-width: min(1lh, 100%) keeps the badges inside the cell and leaves roomy rows unchanged (the count is still a circle); a narrow-container row in the larger-text contract would hold it.
94c6300 to
7fb6e67
Compare
There was a problem hiding this comment.
The Badge fix works where it matters: at 1.25rem and 2.5rem supporting text the pill grows to 33.3px and 66.7px and keeps its label inside, labeled badges are unchanged at the default size, and the new larger-text contract passes in CI. Approval waits on an engineering owner's spec decision: component:Badge FR1 still fixes the block size at --spacing-5, which this change intentionally exceeds, so an owner has to update FR1, and has to approve or reject the draft AST-094 requirements (FR1–FR23, DEC-1, DEC-2) and provide or identify the spec that governs the new typography.dynamicType option and <html data-astryx-text-size>, recording it by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner). Independently of that decision:
min-width: min(1lh, 100%)still lets a badge leave a crowded flex row (44.7px past a 260px row at 2.5rem text), againstcomponent:BadgeFR3; keepingmin-width: 0letsmin-height: 1lhalone fix the spill.- The same minimum widens empty-label badges from 16px to 20px at the default size, which the changeset says does not happen.
- Land the stack through #8053 and #8230 first, or retarget onto #8230, so this diff is the 325-line Badge change, and correct the stack's references to files and to an AST-090 record that do not exist.
[Automated review]
| // one line of its text must still stay inside it and truncate. | ||
| maxWidth: '100%', | ||
| minWidth: 0, | ||
| minWidth: 'min(1lh, 100%)', |
There was a problem hiding this comment.
100% here is the containing block's width, which in a flex row is the whole row, not the space the row's other items leave. At 2.5rem text beside a 230px label that cannot shrink in a 260px row, the count stays 66.7px wide and ends 44.7px past the row, where min-width: 0 keeps it inside, so component:Badge FR3 (“may shrink below its content size”, “The badge stays within its container”) no longer holds there. Keeping min-width: 0 lets min-height: 1lh alone fix the spill; a crowded-row case in the larger-text contract would hold it.
There was a problem hiding this comment.
Confirmed and fixed in be1de76. Measured in Chromium at 402 × 714 with neutral: at 2.5rem text beside a 230px title in a 260px row, min-width: min(1lh, 100%) kept the count 66.7px wide, 44.7px past the row; with min-width: 0 it shrinks to 22px and stays inside. Any width floor that keeps a count circular breaks FR3 the same way in a crowded row, so min-width is back to main's 0 and only min-height: 1lh remains, applied when the badge has a label. The Larger Text story now has a crowded row (a 200px title that cannot shrink beside a count in a 260px row) and a 52px cell, and the larger-text contract asserts the count stays inside both; against the previous head those cases fail (14.7px past at 2.5rem).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Badge: a badge is now never shorter than one line of its own text and never narrower than it is tall. With a theme that follows the reader's text size on iOS (`typography.dynamicType`), a badge's label grew at the larger and accessibility sizes while the badge kept its fixed height, so the label spilled out of the pill and white text on a colored badge became unreadable. The badge now grows with its label, and a one-character count stays at least as wide as it is tall instead of turning into a tall capsule, while a badge in a container narrower than that still stays inside it. At the default size nothing changes, and a theme that sets the badge's height keeps that height there while its badges still grow with larger text. |
There was a problem hiding this comment.
“At the default size nothing changes” does not hold for an empty-label badge: under neutral min-width: min(1lh, 100%) widens it from 16 × 20 to 20 × 20. Either keep min-width: 0 or say so here.
There was a problem hiding this comment.
Confirmed and fixed in be1de76: with min-width back to 0, an empty-label badge is 16 × 20 again at the default size (24 × 30 under Butter). min-height: 1lh now applies only when the badge has a label, so a color-only badge also keeps that size at 1.25rem and 2.5rem instead of growing into a 16 × 67 sliver. Each row of the Larger Text story ends with a color-only badge, the larger-text contract checks its height and width at every size (20 × 20 against 16 × 20 fails it), and the changeset now says a color-only badge keeps its size.
c63ecfe to
be1de76
Compare
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows to 33.3px and 66.7px and keeps its label inside, while default-size, color-only and narrow-container badges are unchanged. Approval waits on an engineering owner's spec decision, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner): component:Badge FR1 still fixes the block size at --spacing-5, so an owner has to update it for this intentional change, and has to approve or reject the draft AST-094 requirements (FR1–FR23, DEC-1, DEC-2) and provide or identify the spec for the stack's typography.dynamicType, theme motion roles and the two experimental subpaths. Independently of that decision:
- Land the stack through #8076 and #8230 (and the pull requests they merge) first, or retarget onto #8230, so this diff is the 386-line Badge change instead of 31,379 authored lines.
- Drop the
platformadaptation condition or add it tospec:AST-012FR2, whose vocabulary is closed. - Reconcile the theme motion roles with AST-094 FR4 and DEC-1, which reject themed touch physics, and fix FR7's reference to a
spec:AST-090that does not exist.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
platform extends the adaptation-rule vocabulary that spec:AST-012 FR2 closes (“Future environment fields require explicit additions to this closed vocabulary”), and no record adds it. Drop it from this stack, or add it to FR2 in an owner-approved record change.
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch | ||
| presentation MUST read its motion from the table on every theme. Themes keep |
There was a problem hiding this comment.
FR4 says a touch presentation reads its motion from the table on every theme, and DEC-1 rejects tuning touch motion per theme, but this diff lets defineTheme({motion: {springs, durations, curves}}) retune those roles and the animator plays the theme's value (packages/core/src/motion/animator.ts:119–:125). One of the two has to change before the record is approved.
be1de76 to
f14b903
Compare
f14b903 to
2d8bdbe
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px and keeps its label inside, while default-size, colour-only and narrow-container badges are unchanged, and the new larger-text contract fails its enlarged-text cases without the fix. Approval waits on an engineering owner's spec decision, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner): component:Badge FR1 still fixes the block size at --spacing-5, so it needs updating for this intentional change, and #8230's typography.dynamicType with <html data-astryx-text-size> needs a governing spec provided or identified, including whether the reader's text size may take the phone base below the 16px that design:mobile-typography DR1 sets. Landing #8230 through its own review first would leave this pull request with only the Badge change. Independently of that decision:
typography.dynamicTypenever scales the literal--text-supporting-size: 12pxthat Butter, Stone and Y2K set, so under those themes supporting text and badge labels stay 12px at every reader size; scale literal--text-*-sizeoverrides too, or move those themes onto the scale and document the limit.
[Automated review]
| * never goes below the step's own size, so the theme's sizes at Large stay | ||
| * exactly its own. | ||
| */ | ||
| export function scaledFontSizeEntries( |
There was a problem hiding this comment.
Only the twelve raw --font-size-* steps are rescaled here, so a semantic size that a theme sets as a literal never follows the reader. Butter, Stone and Y2K set '--text-supporting-size': '12px', so a theme extending them with typography: {dynamicType: true} keeps supporting text and every Badge label at 12px at every text size while body text grows about threefold at AX5, against the docs' “every font size follows the reader's choice”. Scaling a literal --text-*-size override on the curve of the step its core default reads (--text-supporting-size reads --font-size-sm, Footnote), or moving those themes onto the scale and documenting the limit, would close it.
There was a problem hiding this comment.
Fixed in #8230 (merged into this branch): a semantic size a theme fixes at a length now scales on the curve of the step its core default reads, so '--text-supporting-size': '12px' scales like Footnote; a size that reads a variable, em or % follows its source and is never scaled twice. The Dynamic Type CSS comes after the theme's own rules, so each adaptation rule's block re-declares the sizes it sets through a variable once an earlier block scaled them, and Butter and Stone's phone rule keeps its step on phones.
Measured on the iOS 26.2 simulator, Core/Themes/Dynamic Type › Fixed Supporting Size (Y2K with dynamicType): at AX5, supporting text is 40.6px (12 × 44/13) beside 49.9px body, where it stayed 12px before; the badges on that page go from 30px tall with 12px labels at Large to 62px tall with 40.6px labels at AX5. Three tests that failed before cover the root, each adaptation rule's block, and sizes that read a variable or their parent.
2d8bdbe to
0d536f0
Compare
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px and keeps its label inside, default-size and color-only badges are unchanged, and #8230 now scales the 12px supporting size that Butter, Stone and Y2K fix. Approval waits on an engineering owner's spec decision, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner): component:Badge FR1 still fixes the block size at --spacing-5, so it needs updating for this intentional change, and #8230's typography.dynamicType with <html data-astryx-text-size> needs a governing spec provided or identified, including whether the reader's text size may take the phone base below the 16px that design:mobile-typography DR1 sets. Landing #8230 through its own review first would leave this pull request with only the Badge change.
[Automated review]
0d536f0 to
9f1c619
Compare
There was a problem hiding this comment.
The Badge fix works and the Dynamic Type stack follows spec:AST-102, which is now current: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px with its label inside, and default-size and color-only badges are unchanged. Three changes are needed before approval:
- Keep
!importantoutside the scaling expression:'--text-supporting-size': '12px !important'now compiles tocalc(12px !important * …), which browsers reject, so that text loses the theme's size everywhere (packages/core/src/theme/dynamicType.tslines 221 and 286). - Remove
typography.dynamicTypefrom the Badge changeset, since that option does not exist. - Add a macOS Safari check to the description showing the theme's own sizes there, as
spec:AST-102FR3's verification asks.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Badge: a badge with a label is now never shorter than one line of that label. With a theme that follows the reader's text size (`typography.dynamicType`), a badge's label grew at the larger and accessibility sizes while the badge kept its fixed height, so the label spilled out of the pill and white text on a colored badge became unreadable. The badge now grows with its label. At the default size nothing changes, a theme that sets the badge's height keeps that height there while its badges still grow with larger text, a color-only badge keeps its size at every text size, and a badge still shrinks and truncates in a container too narrow for it. |
There was a problem hiding this comment.
No typography.dynamicType option exists at this head: the theme tests assert defineTheme({typography: {dynamicType: true}}) is a type error, and spec:AST-102 FR3 rules one out. Something like “When text follows the reader's text size, as every theme does on iPhone and iPad” keeps the point without sending readers to an API they cannot use.
There was a problem hiding this comment.
Reworded in 87946a5: "When text follows the reader's text size, as every theme does on iPhone and iPad, a badge's label grew…". It names no option.
| .filter(name => tokens[name] !== undefined) | ||
| .map(name => { | ||
| const value = tokens[name]; | ||
| const scaled = `calc(${value} * var(${dynamicTypeScaleProperty(FONT_SIZE_STEP_STYLES[name])}, 1))`; |
There was a problem hiding this comment.
This wraps the whole authored value, so a step written as '1rem !important' becomes min(calc(1rem !important * var(--_dynamic-type-body, 1)), max(var(--font-size-lg), 1rem !important)), and readerTextSizeTokens does the same to '12px !important' (line 286). !important inside calc() makes the value invalid where it is used, so the text falls back to its parent's size (20px instead of 12px in Chromium) in every browser, while the base emits these tokens as written. Splitting off a trailing !important before scaling and appending it after the expression keeps both the size and the importance.
There was a problem hiding this comment.
Fixed at origin in #8230 (082e89e), which this branch now merges. The scaling splits off a trailing !important, scales and order-checks the bare value, and puts the importance back after the whole expression, for raw steps and for fixed sizes alike: '12px !important' now emits calc(12px * var(--_dynamic-type-footnote, 1)) !important. A test that failed before covers both paths, plus the order check across an important step. In Chromium 149, that theme inside a 20px parent now computes supporting text at 12px and the step at 16px; the previous head gave 20px for both.
| * `font: -apple-system-body`, as every browser on iPhone and iPad does. There | ||
| * is no platform, touch-callout or pointer check, since touch behavior is one | ||
| * on every touch device and this is a style the engine itself provides. | ||
| * macOS Safari knows the keyword too, but its Body is a fixed 13px that names |
There was a problem hiding this comment.
This is the only evidence for macOS Safari, and the jsdom test fakes the 13px. spec:AST-102 FR3's verification lists macOS Safari as a representative state, so a measurement of the probe and the Dynamic Type story in macOS Safari would back it.
There was a problem hiding this comment.
Not measured: no Mac was available to the test rig, so both descriptions now say "not measured on macOS Safari" instead of claiming it. The no-category path (Safari's fixed 13px Body names no category, so every scale stays 1) is covered only by the jsdom test, which fakes that size. A real macOS Safari measurement is still owed.
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px with its label inside, and default-size and colour-only badges are unchanged, but three changes are needed before approval:
- Land #8230 first and then update this branch from
main, so this pull request carries only the 371-line Badge change instead of 2,008 lines; updating also clears thepr-interactionfailure. - In #8230, scale only fixed-length font-size steps, because a theme step written as
var(--font-size-base)now forms a reference cycle and that text loses the theme's size in every browser (packages/core/src/theme/dynamicType.tslines 218–233). - Add a macOS Safari measurement of the text-size probe and the Dynamic Type story, which
spec:AST-102FR3's verification lists.
[Automated review]
| .filter(name => tokens[name] !== undefined) | ||
| .map(name => { | ||
| const [value, important] = splitImportant(tokens[name]); | ||
| const scaled = `calc(${value} * var(${dynamicTypeScaleProperty(FONT_SIZE_STEP_STYLES[name])}, 1))`; | ||
| const next = steps[steps.indexOf(name) + 1]; | ||
| if (next === undefined) { | ||
| return [name, scaled + important]; | ||
| } | ||
| const own = lengthInPx(value); | ||
| const nextOwn = lengthInPx(splitImportant(tokens[next] ?? '')[0]); | ||
| const inOrder = own !== null && nextOwn !== null && own <= nextOwn; | ||
| return [ | ||
| name, | ||
| (inOrder | ||
| ? `min(${scaled}, var(${next}))` | ||
| : `min(${scaled}, max(var(${next}), ${value}))`) + important, |
There was a problem hiding this comment.
This scales and caps every step a theme writes, whatever its value. A theme with a type scale that sets '--font-size-lg': 'var(--font-size-base)' now gets --font-size-base: min(…, max(var(--font-size-lg), 0.875rem)) beside --font-size-lg: min(calc(var(--font-size-base) * …), max(var(--font-size-xl), var(--font-size-base))), a reference cycle: in Chromium both compute to nothing and the text falls back to its parent's 16px instead of 14px, with no reader size set. Scaling only fixed-length steps (as readerTextSizeTokens does with isFixedSize) and capping by the next step only when its value is a fixed length would keep such themes on their own sizes.
There was a problem hiding this comment.
Fixed at origin in #8230 (d7d6ba5), which this branch now merges. Each step a block writes at a fixed length keeps its scaled size in a private --_scaled-font-size-* property, and the step reads it. Caps chain only between those private properties, which only the scaling writes, so no cap can read back a variable a theme wrote, in any block. That also closes a cross-block case the narrower rule would leave open: a root ramp plus an adaptation rule that writes --font-size-lg: var(--font-size-base). A step written as a variable is emitted as written and follows what it reads, so nothing scales twice. Tests that failed before cover the alias at the root, the alias in a rule over a root ramp, and an acyclic check over every step aliased at the root and in a rule. In Chromium 149, your 14/1.2 theme in a 16px parent with no reader size now computes body and large text at 14px and small text at 12px; all three were 16px before. The rule case matches.
| * `font: -apple-system-body`, as every browser on iPhone and iPad does. There | ||
| * is no platform, touch-callout or pointer check, since touch behavior is one | ||
| * on every touch device and this is a style the engine itself provides. | ||
| * macOS Safari knows the keyword too, but its Body is a fixed 13px that names |
There was a problem hiding this comment.
This is the only statement of what happens in macOS Safari, and the jsdom test behind it fakes the 13px Body. spec:AST-102 FR3's verification lists macOS Safari as a representative state, so a measurement there of the probe's computed size and of the Dynamic Type story would back it.
There was a problem hiding this comment.
Same as before: not measured, because no Mac was available to the test rig. Both descriptions say "not measured on macOS Safari" instead of claiming it, and the jsdom test, which fakes the 13px Body, is the only coverage of that path. The measurement is still owed.
| * the theme's own size at Large, a theme emits the scaled sizes only when it | ||
| * opts in (root and adaptation rules, behind the capability alone), and the root sync | ||
| * writes and clears `<html data-astryx-text-size>` |
There was a problem hiding this comment.
This header still describes an opt-in and a root sync that writes <html data-astryx-text-size>; the tests below assert that neither exists. Rewording it to the no-option, no-attribute behaviour keeps it accurate.
There was a problem hiding this comment.
Reworded in d7d6ba5. The header now describes the current behaviour: every theme writes its sizes that way at its root and in its adaptation rules, with no option and no reference cycle, and the root sync reads a hidden probe and adopts a stylesheet, writing nothing on <html>.
|
On landing #8230 first: keeping this branch on #8230 until it merges is what lets the Badge fix be tested at the text sizes it fixes. Once a maintainer merges #8230, this branch updates from |
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px with its label inside, and default-size and colour-only badges are unchanged in every theme, but two changes are needed before approval:
- Land #8230 first and then update this branch from
main, so this pull request carries only the 371-line Badge change instead of 2,297 authored lines. - Measure the text-size probe and the Dynamic Type story in macOS Safari (in #8230 if it lands first), because
spec:AST-102FR3's verification andspec:AST-013FR10 call for that evidence behind the code's macOS claim.
[Automated review]
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px with its label inside, and default-size and colour-only badges are unchanged in every theme, but three changes are needed before approval:
- Land #8230 first and then update this branch from
main, so this pull request carries only the 371-line Badge change instead of 2,310 authored lines. - Measure the text-size probe and the Dynamic Type story in macOS Safari (in #8230 if it lands first), because
spec:AST-102FR3's verification andspec:AST-013FR10 call for that evidence behind the code's macOS claim. - Scale the font sizes a theme sets in
onDarkandonLightas well:generateOnMediaCSSwrites them withoutreaderTextSizeEntries, so text on aMediaThemesurface ignores the reader's text size.
[Automated review]
| // gives the page its scales (see dynamicType.ts); elsewhere each scale is | ||
| // 1, so the values are the theme's own. | ||
| joinDeclarations( | ||
| readerTextSizeEntries(tokenEntries), |
There was a problem hiding this comment.
Only tokenBlock routes through readerTextSizeEntries; generateOnMediaCSS writes a theme's onDark and onLight tokens straight through joinDeclarations (:1697–:1707), so onDark: {tokens: {'--font-size-base': '15px'}} compiles to [data-astryx-media="dark"] { … --font-size-base: 15px; } and text on that MediaTheme surface ignores the reader's text size (spec:AST-102 FR3). Passing the on-media entries through readerTextSizeEntries as well, with an onDark case in dynamicType.test.ts, would close it.
There was a problem hiding this comment.
Fixed at origin in #8230 (82390ce), which this branch now merges. A media surface's token entries now go through readerTextSizeEntries, as the root and adaptation blocks do: onDark: {tokens: {'--font-size-base': '15px'}} compiles to the scaled private size plus --font-size-base: var(--_scaled-font-size-base) on [data-astryx-media="dark"], and a size fixed in onLight scales on its step's curve. A test that failed before covers both.
There was a problem hiding this comment.
The Badge fix works: at 1.25rem and 2.5rem supporting text a labeled badge grows from 20px to 33.3px and 66.7px with its label inside, while default-size and colour-only badges are unchanged under neutral and Butter. Two changes are needed before approval:
- Land #8230 first and then update this branch from
main, so this pull request carries only the 371-line Badge change instead of 2,345 authored lines. - Measure the text-size probe and the Dynamic Type story in macOS Safari (in #8230 if it lands first), because
spec:AST-102FR3's verification andspec:AST-013FR10 call for that evidence behind the code's macOS claim.
[Automated review]
|
We can't capture on macOS, so there are no macOS Safari screenshots. What this PR has: iOS 26.2 Safari on the simulator (the Dynamic Type story at Large, AX3 and AX5, and the Fixed Supporting Size story) and Chromium (the Larger Text story at the default size, 1.25rem and 2.5rem, under neutral and Butter). On macOS, #8230's fixed 13px body size is covered only by a jsdom test. Whether that's enough is up to you. |
A badge held a fixed height, the theme's --spacing-5, so when its label's text grew (a larger text size) the label spilled out of the pill and white text on a colored badge left the color behind. A badge with a label keeps main's height and gains min-height: 1lh, so the theme's height (Butter's 30px included) holds at the default size and the badge grows past it when the text needs more room. A color-only badge has no text and keeps its size at every text size, and there is no width floor: a badge still shrinks and stays inside a container narrower than one line (component:Badge FR3).
6c34304 to
a774d6f
Compare
There was a problem hiding this comment.
This Badge fix is ready from a review standpoint: a labeled badge now grows with its text (20px to 33.3px and 66.7px at 1.25rem and 2.5rem supporting text under neutral), every default-size badge is unchanged across the themes, and the new Chromium contract fails its grow cases without the fix. The change matches component:Badge FR1 and spec:AST-102 FR4 as they stand on main, so no spec decision is needed. Next step: mark the pull request ready for review so CI runs on this commit.
[Automated review]
cixzhang
added
the
ci:run
User impact
A
Badgeholds a fixed height (--spacing-5, 20px in the neutral theme, or a theme's own, such as Butter's 30px). When its label's text grows with a larger text size, the label outgrows the pill: it spills out, and white text on a colored badge leaves the color behind and becomes unreadable.Problem and solution fit
A badge with a label is now never shorter than one line of that label (
min-height: 1lh). The minimum doesn't bind at the default size, so nothing moves there; at larger sizes the pill grows with its label.Expected behavior and authority
component:BadgeFR3: a badge is one line, and its label never escapes its container. WCAG 1.4.4 (Resize Text): text can grow without losing content.Smallest restoration
min-height: 1lhon a badge with a label, while main's height stays: a theme's height (Butter's 30px included) holds at the default size.component:BadgeFR1).Evidence
BadgeLargerText.a11y.chromium.spec.tsfails 3 of its 10 cases with main'sBadge.tsx(the badge growing with its label at 1.25rem and 2.5rem text, under the neutral theme and Butter) and passes 10 of 10 with this change.iOS simulator, the Larger Text story, main's Badge above this pull request:
Scope
Testing
Local gates at a774d6f (on main 9ee7457):
pnpm check:repo,pnpm check:changesets,pnpm typecheck(0 errors), ESLint and Prettier on the touched TypeScript and Markdown files, the root build and the Storybook build pass.pnpm test:ui: 16,454 tests pass.pnpm test:node: 14,563 pass; its 4 failures, instory-tree.test.tsand the releasecache.test.mjs, are independent of this change (test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509, fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520).BadgeLargerText.a11y.chromium.spec.ts: 10 of 10 in Chromium, twice.