Repository navigation
Conversation
|
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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review is waiting on a spec decision: every code change here rests on the draft spec:AST-094 (FR1–FR3 in particular), so an engineering owner needs to approve or reject its proposed requirements, including its proposed amendments of the theme-tokens architecture, the user-states design and AST-025 FR14. That decision is recorded by an engineering owner commenting /approve-spec <full head SHA> on this pull request, and the author can record it as an engineering owner. Whatever the decision, three fixes are needed:
- Remove or alias
springs.switchThumbMoveandsprings.switchThumbStretch(packages/core/src/motion/uikit/springs.ts:279-297): they are WebKit's values, now contradict the measured springs inswitch.ts, and miss UIKit's measured switch frames by up to 3.3 pt (travel) and 9.5 pt (lift). - Land #8052–#8054 first and update this branch, so its diff is its own six files instead of 4,698 lines across the stack; its copy of AST-094 already predates #8052's revised FR8 and FR19.
- Correct
MEASUREMENTS.md, which points at auikit/contextMenu.tsthat is not in this tree and leaves several switch captures out of its open list.
[Automated review]
| * (RenderThemeIOS.h) | ||
| * @measure M-SW02 | ||
| */ | ||
| switchThumbMove: {response: 0.3, dampingRatio: 0.75}, |
There was a problem hiding this comment.
switchThumbMove (0.3 s, 0.75) and switchThumbStretch below are WebKit's values, but switch.ts now records UIKit's measured travel (0.3 s, critically damped), lift and settle, and MEASUREMENTS.md marks M-SW02 and M-SW03 done. Against the UIKit travel frames in switchTouch.test.ts, this spring misses by up to 3.3 pt and overshoots the on end by 0.6 pt. Remove both entries, or alias them to switchControl.travel, .lift and .settle.
There was a problem hiding this comment.
These two entries are not part of this pull request's own change: they come from the stacked springs PR that owns springs.ts, where they stay as WebKit's iOS 18 switch springs that other tests read. Switch reads the measured switchControl table this PR adds and never these two. Aliasing them to switchControl or removing them is a call for that PR; I have passed it on.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsSwitch (@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: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
6b99705 to
00aa208
Compare
|
Replies to the automated review:
|
There was a problem hiding this comment.
This review is waiting on a spec decision: the UISwitch values, flip rule and switch motion roles rest only on the draft spec:AST-094, so an engineering owner needs to approve or reject its FR1–FR4, best on #8052's current text, because this branch's copy is older and its FR4 forbids the themeable touch motion this code adds. That decision is recorded by an engineering owner commenting /approve-spec <full head SHA> on this pull request, and the author can record it as an engineering owner. Whatever the decision, these fixes are needed:
- Make the compact-touch test in
utils/touchPresentation.tsfollowspec:AST-012FR9 (width below the theme'smdpoint or height below 500) instead of a 744 px constant, with tests for 767/768 px and an overriddenmd, and correct the menu docs and changeset. - Land the stacked predecessors first and update this branch, so its diff is its own eight files instead of 29,027 authored lines.
- Remove the
platformadaptation condition, or have it added tospec:AST-012FR2's closed vocabulary first. - Tag
uikit.switchControl@experimentaland file its changeset as[experimental], and remove or alias the duplicatesprings.switchThumbMoveandsprings.switchThumbStretchroles.
[Automated review]
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
spec:AST-012 FR9 (current) defines compact as a viewport width below the theme's effective md point (768 by default) or a height below 500, and says no resolver keeps size numbers of its own; DEC-6 rejects mapping compact to platform size classes. With 744 here, a coarse-pointer screen 744–767 px wide and at least 500 tall gets the popover where FR9 gives the sheet, and a theme that moves md changes nothing. Taking the width edge from the theme's md point keeps presentation="adaptive" on the five menus and selectors on FR9.
| * 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.
spec:AST-012 FR2 limits when to width, height, pointer, contrast and motion, and says "Future environment fields require explicit additions to this closed vocabulary." No record adds platform yet, so this condition needs AST-012's owners to add it first, or it should leave this stack.
|
|
||
| import {registerMotionTable} from '../roles'; | ||
|
|
||
| export const switchControl = { |
There was a problem hiding this comment.
This table now reaches @astryxdesign/core/motion/experimental as uikit.switchControl (uikit/index.ts:35, motion/experimental.ts:65). spec:AST-017 FR21 asks that every exposed declaration carry @experimental, as springs, scroll, timing, navigation and the gesture tables do; this one does not, and motion/experimental.ts's uikit doc comment does not list it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [chore] Internal: UISwitch's values measured on iOS 26.2 (its geometry, the springs of the thumb's lift, settle, travel and drag, the track colour, and its flip distance) join the UIKit constants table, with the rule a switch's drag follows as a pure function: the finger flips the switch once it has travelled the track width minus the thumb width from where it landed, coming back past that point flips it back, and lifting the finger commits the side shown, or toggles when nothing flipped. Its springs and its minimum lift time are motion roles a theme can retune (`switchControl.lift`, `settle`, `travel`, `tracking`, `wellOn`, `wellOff` and `minimumLiftMs`), listed in the role table of `astryx docs motion`. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
switchControl is added to the experimental uikit namespace by this pull request, so spec:AST-017 FR22 calls for an [experimental] changeset; [chore] with "Not exported from the package entry point" undersells a surface product builders can import.
…a fixed time The spec's story has a bottom-sheet TimeInput with no value, so its Minute wheel opens on the current minute, and the minute list ends at 59. The one-finger case drags the wheel up 120 px and asserted that it scrolled more than 60 px, so a run in the last minutes of an hour failed: from :57 the column can scroll only two 28 px rows before it reaches row 59 (1596 to 1652, the pr-a11y failure on meta#8123), and from :59 none at all. The spec now pins the clock to 10:30 before the story loads, polls scrollTop instead of sleeping 600 ms, and asks for at least one row, measured from the rendered option.
00aa208 to
bcccef2
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
This review is waiting on a spec decision: the UISwitch values, flip rule and switch motion roles rest only on the draft spec:AST-094, whose current text now lives in #8316, while this branch's copy still says touch motion is not themeable (FR4). An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, these fixes are needed:
- Land the stacked predecessors first and update this branch, so its diff is its own eight files instead of 29,685 authored lines.
- Remove the
platformadaptation condition, or have it added tospec:AST-012FR2's closed vocabulary first. - Make the touch resolver follow the theme's effective
mdpoint (spec:AST-012FR9) with an overridden-mdtest, or narrow its comment and changeset to the default point, and correct the gestures README, which still says 744 px. - Tag
uikit.switchControl@experimental, file its changeset as[experimental], and remove or alias the duplicatesprings.switchThumbMoveandsprings.switchThumbStretchroles.
[Automated review]
| export const COMPACT_QUERY = DEFAULT_COMPACT_DEFINITION.query; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| DEFAULT_COMPACT_DEFINITION.coarsePointerQuery; |
There was a problem hiding this comment.
COMPACT_QUERY and COMPACT_TOUCH_QUERY are built once from DEFAULT_COMPACT_DEFINITION, so a theme that moves md never moves this resolver. spec:AST-012 FR9 defines compact by "the effective md point (FR1)" and lists "overridden md" among its verification states: with md at 900, a coarse-pointer 850 × 1200 screen gets the popover where FR9 gives the sheet. compactDefinition(points) already exists for a theme's effective points; derive the queries from it and add an overridden-md test, or narrow this comment and .changeset/touch-presentation.md to the default point.
| - layout behaviours key on a compact viewport, narrower than 744 px or | ||
| shorter than 500 px (`COMPACT_QUERY`), the closest a size can get to | ||
| UIKit's compact size class (a Max-size iPhone in landscape is its one | ||
| known miss); |
There was a problem hiding this comment.
This still describes the 744 px size-class approximation the resolver no longer uses: COMPACT_QUERY is now the default compact definition (width below 768 or height below 500), and spec:AST-012 FR9 says compact is not mapped to platform size classes.
…a fixed time (#8332) ## Maintainer impact The picker wheel pinch-zoom contract in `pr-a11y` fails in the last minutes of an hour, on PRs that don't touch the wheel. On #8123 (run 40137962720, job 121010245944) it expected scrollTop > 1656 and got 1652. ## Intended invariant The one-finger case proves that one finger still scrolls a picker wheel and leaves the page at scale 1 (WCAG 1.4.4); the pinch case beside it proves the zoom. Neither depends on the time of day. ## Change The spec's story has a bottom-sheet TimeInput with no value, so its Minute wheel opens on the current minute, and the minute list ends at 59. The one-finger case drags the wheel up 120 px and asserted that it scrolled more than 60 px. Its rows are 28 px, so from :57 the column can scroll only two rows before it reaches row 59: 57 × 28 = 1596 to 59 × 28 = 1652, the failure above. From :59 it can't scroll at all. The spec now: - pins the clock to 10:30 before the story loads (`page.clock.setFixedTime`, in a `beforeEach`), so the wheel opens on :30 with rows to go; - polls scrollTop instead of sleeping 600 ms; - asks for at least one row, measured from the rendered option. Only the spec changes. ## Evidence - Failure before the change: the run above, which opened at :57. Reproduced locally by pinning the clock to 10:59: received 1652 against ≥ 1680. - Success after the change: with the clock at 10:30, both cases pass 16 of 16 in Chromium on the spec's phone profile (`--repeat-each 8`). - Product/runtime behavior verified unchanged: no source, story or style file changes. ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing ``` pnpm --filter ./apps/storybook build ASTRYX_STORYBOOK_DIR=apps/storybook/dist pnpm exec playwright test \ packages/core/src/DateInput/__tests__/WheelPinchZoom.a11y.chromium.spec.ts \ --project chromium --repeat-each 8 ``` 16 passed (42 s). With the clock pinned to 10:59 instead, the one-finger case fails as on #8123.
bcccef2 to
8885cec
Compare
8885cec to
2c9fa07
Compare
There was a problem hiding this comment.
Changes are needed before this can merge: the UISwitch values and flip rule follow the current spec:AST-094, but the branch still carries its whole open stack and keeps two springs for each switch thumb motion.
- Land the stacked predecessors in the stated order and update this branch from
main, or retarget it, so the diff holds only this pull request's own commits (658 authored lines instead of 22,837). - Retire or alias WebKit's
springs.switchThumbMoveandsprings.switchThumbStretch, which sit beside the measuredswitchControl.travel,liftandsettle(spec:AST-094FR1), and drop their@measuretags now that those captures have landed. - Reword the FR4 citation in
switch.tsandswitchTouch.test.ts: FR4 makes every spring a touch presentation plays a theme motion role, so these values stay internal only until Switch's own record names its roles.
[Automated review]
| lift stays, the stretch goes), M-SW16 (iOS 26.2). `springs.switchThumbMove` | ||
| and `switchThumbStretch` still keep WebKit's values. Partly: M-SW07 (a single |
There was a problem hiding this comment.
With M-SW02 and M-SW03 landed in uikit/switch.ts, keeping springs.switchThumbMove (0.3 s, 0.75) and switchThumbStretch (0.35 s, 0.87) leaves the table with two values for the same thumb motions, which spec:AST-094 FR1's value check fails (“two values for one use”), and motion/README.md asks a landed measurement to replace the value and drop its @measure tag. Remove the two entries or alias them to switchControl.travel, lift and settle, and re-base springs.segmentedThumb's guess on the measured travel.
| * Every value here stays an internal constant: no public API retunes a | ||
| * component's measured spring (`spec:AST-094` FR4's roles are the shared | ||
| * ones), and the geometry, rubber band, flip distance, shadow and haptic are | ||
| * not motion. |
There was a problem hiding this comment.
spec:AST-094 FR4 says the reverse of this parenthetical for anything a presentation plays: every spring, duration and curve it plays “MUST be a theme motion role named for its use”, “further roles a component's own record measures join through the generated motion role table (DEC-5)”, and a component “MUST NOT play a literal”. Nothing plays these values yet, so they can stay internal for now; say they stay internal until component:Switch names its roles, rather than citing FR4 for it.
| it('are internal constants, not motion roles', () => { | ||
| // No public API retunes a component's measured spring (spec:AST-094 | ||
| // FR4): the springs and the minimum lift time play as measured. |
There was a problem hiding this comment.
FR4 does not say that no public API retunes a component's measured spring, and “play as measured” is the literal FR4 forbids once a presentation plays these values. Keep the present-state assertion if it helps, but drop the FR4 attribution; the comment at line 193 (“A theme's minimum lift time (the role's resolved value) replaces it”) also contradicts this test.
There was a problem hiding this comment.
Changes are needed before this can merge: the UISwitch values and flip rule follow the current spec:AST-094, but the branch still carries its whole open stack and keeps two springs for each switch thumb motion.
- Land the stacked predecessors in the stated order and update this branch from
main, or retarget it, so the diff holds only this pull request's own commits (659 authored lines instead of 20,491). - Retire or alias WebKit's
springs.switchThumbMoveandsprings.switchThumbStretch, which sit beside the measuredswitchControl.travel,liftandsettle(spec:AST-094FR1), and drop their@measuretags now that those captures have landed. - Reword the FR4 citation in
switch.tsandswitchTouch.test.ts: FR4 makes every spring a touch presentation plays a motion role, so these values stay plain constants only until Switch's own record names its roles.
[Automated review]
| * Every value here stays an internal constant: no public API retunes a | ||
| * component's measured spring (`spec:AST-094` FR4's roles are the shared | ||
| * ones), and the geometry, rubber band, flip distance, shadow and haptic are | ||
| * not motion. |
There was a problem hiding this comment.
FR4 reads the other way for anything a presentation plays: “Every spring, duration and curve a touch presentation plays MUST be a motion role named for its use”, with a component's own roles joining “through the generated motion role table (DEC-5)”; DEC-11 is what keeps every role away from themes for now. Suggest saying these values stay internal constants until component:Switch names its roles under FR4 and DEC-5.
There was a problem hiding this comment.
Reworded in d45e529: plain constants until component:Switch names its roles (spec:AST-094 FR4, DEC-5); DEC-11 keeps every role from themes.
| // No public API retunes a component's measured spring (spec:AST-094 | ||
| // FR4): the springs and the minimum lift time play as measured. |
There was a problem hiding this comment.
FR4 does not say this: it requires a spring a presentation plays to be a motion role and forbids playing a literal. Pinning “not motion roles” holds while nothing plays these values, but the FR4 attribution points the presentation toward literals; the theme note at line 193 is also stale under DEC-11.
There was a problem hiding this comment.
Reworded in d45e529: plain constants until component:Switch names its roles (spec:AST-094 FR4, DEC-5). The settleTime case now names the caller's minimum, not a theme's.
| lift stays, the stretch goes), M-SW16 (iOS 26.2). `springs.switchThumbMove` | ||
| and `switchThumbStretch` still keep WebKit's values. Partly: M-SW07 (a single |
There was a problem hiding this comment.
With M-SW02 and M-SW03 captured, the motion README's step 4 says to replace the value and drop the @measure tag. Keeping WebKit's switchThumbMove and switchThumbStretch beside switchControl.travel, lift and settle leaves two values for one use, which spec:AST-094 FR1's value check fails; alias or remove them in springs.ts.
There was a problem hiding this comment.
Done in ca7f472: SWITCH_THUMB_MOVE_SPRING holds the measured travel, which the switch table reads; WebKit's stretch spring is retired, with both @measure tags.
…ta#8270's heads The three bases of the Switch touch branch in one commit, with nothing of its own: the uikit barrel's header names both sides' domains. The branch merges this commit so a compare from it shows only the branch's own change.
There was a problem hiding this comment.
Changes are needed before this can merge: the UISwitch values, the flip rule and the one measured thumb travel follow the current spec:AST-094, but the branch still carries its whole open stack.
- Land the stacked predecessors in the stated order and update this branch from
main, or rebase its own commits ontomain(all but one README row apply as they are), so the diff holds its own 704 lines instead of 20,536. - Re-base
springs.segmentedThumb's guess on the measured switch travel, or say it keeps WebKit's value on purpose, since the WebKit switch spring it names is gone from the table.
[Automated review]
| export const SWITCH_THUMB_STRETCH_SPRING = { | ||
| response: 0.35, | ||
| dampingRatio: 0.87, | ||
| dampingRatio: 1, |
There was a problem hiding this comment.
This row is now UIKit's critically damped travel, but SEGMENTED_THUMB_SPRING just below (:389-399) still keeps 0.3 s / 0.75 as a guess from “WebKit's UIKit-matched switch thumb spring, the nearest sourced control thumb”, a spring the table no longer holds. Re-base the guess on this row, or say it keeps WebKit's value on purpose.
…it constants table UISwitch, measured on iOS 26.2 (the switch table, motion/uikit/switch.ts): the 63 × 28 track and the 37 × 24 thumb lifted to 58 × 38.33; the lift, settle, travel and drag springs; the 235 ms minimum lift; the drag's rubber band and flip distance; the track colour's springs; no haptic. The springs table's SWITCH_THUMB_MOVE_SPRING holds the measured travel the switch table reads, WebKit's stretch spring goes, and the segmented thumb's guess cites WebKit's spring itself until UISegmentedControl's measured spring replaces it. Switch/switchTouch.ts: UIKit's proportions in any track, the thumb under a dragging finger, the flip rule (the finger flips the switch once it has travelled the track minus the thumb from where it landed), the commit at the lift and the settle time, as pure functions. Every value stays an internal constant with no theme surface; no component plays them yet.
ca7f472 to
64404bb
Compare
github-actions
Bot
removed
the
needs:design-review
|
Rebased onto main as one commit, as suggested: the pull request now shows only its 8 files. |
There was a problem hiding this comment.
This looks ready to merge: UISwitch's measured values and the switch's drag rule follow the current spec:AST-094 FR1–FR3 as internal constants and pure functions, and the branch now carries only its own change. The new tests hold the springs, the rubber band and the flip rule to the UIKit readings they cite, and they pass along with the motion and Switch suites, typecheck and lint.
[Automated review]
meta#8123 is now one commit on main 42a30b4, which this branch already carries. Its files are this branch's already, then changed here (the switch touch module, its test, the changeset), so the merge keeps this branch's side for them and for the motion README and the uikit barrel; the segmented thumb's guess takes meta#8123's wording.
Only motion/README.md's Files table needed a hand: main's table, with this pull request's uikit/switch.ts row after uikit/gestures.ts.
There was a problem hiding this comment.
This looks ready to merge: UISwitch's measured values and the switch's drag rule follow the current spec:AST-094 FR1–FR3 as internal constants and pure functions, and the new tests hold the springs, the rubber band and the flip rule to the UIKit readings they cite. Nothing more is needed here; before #8124 plays travel for a programmatic change, that use needs M-SW17's measurement or a guess label naming it, since no capture covers it yet (packages/core/src/motion/uikit/switch.ts:98-99).
[Automated review]
| minimumLiftMs: 235, | ||
| /** | ||
| * The thumb travelling to the committed end after a release (a tap, a | ||
| * hold, a drag) and on a programmatic change, from the thumb's own |
There was a problem hiding this comment.
No capture covers this use yet: MEASUREMENTS.md keeps M-SW17 ("is the programmatic animation the tap animation?") open. Before a presentation plays travel for a programmatic change, add @measure M-SW17 and say that use is a guess, as minimumLiftMs does for M-SW07, or drop the clause (spec:AST-094 FR2).
Applies to main by itself. Review only this PR's change: vjeux/astryx@2c4f172...uikit/c02-switch-values
Why
Switch's touch presentation (#8124) has to move like iOS's switch, and guessed numbers show: UISwitch flips once the finger is 26 pt past where it landed, wherever it landed, not when the thumb crosses the middle.
What led to it
spec:AST-094FR1–FR3 (current) require touch presentations to use UIKit's measured, sourced values on UIKit's physics. The UIKit constants table had nothing for UISwitch.Options and tradeoffs
spec:AST-094DEC-3 keeps gesture thresholds out of themes and DEC-11 keeps touch motion roles internal.Why this choice
Option 3 is what FR1–FR3 ask for, and it adds no API (the changeset is
[chore] Internal). Two measurements set the details:How to check it
Nothing changes on screen by itself; #8124 uses these values.
pnpm exec vitest run --project ui packages/core/src/motion packages/core/src/Switch/switchTouch.test.ts.switchTouch.test.tsreplays UISwitch's recorded drags (from x 8, 20.5 and 45, with and without a 400 ms hold, mirrored when on) and expects each flip on the same touch event as UIKit; it holds the lift, settle and travel springs to UIKit's frames.uikit.test.tschecks that every new table entry names its source and that no switch value is a theme motion role.Evidence
Read from UISwitch on the iOS 26.2 simulator: its tracking state on every frame under 1 pt drags. The springs match UIKit's frames within 0.7 pt (lift) and 0.45 pt (settle, travel);
motion/MEASUREMENTS.mdlists the sources.Scope