Repository navigation
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 PreviewNo new or modified components detected. 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 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides on spec:AST-094: the new datePicker table rests on its FR1 and FR2, but the record is a draft this branch adds (approved_by: null, not yet on main). An engineering owner records that decision by commenting /approve-spec 7931cc3be44b0d85c178c5208d840e0f6cb3426e on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch carries its unmerged stack (#8043-#8045, #8052-#8056, #8067), 13,285 authored lines for a 351-line change, so land those first or rebase so the diff is this commit alone.
lintruns out of JavaScript heap ineslint .on this head, as on every stack head from #8045 on, while it passes onmain.datePicker.tsnamespaging.release,paging.integrator,paging.animatedScrolland apickertable that only #8145 and #8156 add, so stack on those or drop the references.- The description needs the
## Writing rubric resultsthat a pull request creating a record must report.
[Automated review]
| * A release picks its month by the paging rule and settles on the | ||
| * paging integrator (`paging.release`, `paging.integrator`): slow | ||
| * drags released at 30 % and 50 % of a page settle back, at 70 % on | ||
| * the next month; a drag toward a month past `maximumDate` stays. | ||
| * @source measured: UIKit lab, iOS 26.2 simulator, `.date` `.inline`, | ||
| * the grid's `UICollectionView` (`isPagingEnabled`, | ||
| * `decelerationRate` 0.998) read and its offsets logged per frame | ||
| * for slow drags released at 30, 50 and 70 % and a drag past the | ||
| * last allowed month, Oct 23 2026 | ||
| */ | ||
| release: 'paging', |
There was a problem hiding this comment.
paging.release and paging.integrator do not exist at this head: motion/uikit has no paging domain, and scroll.paging holds only projectionRate (a GUESS of 0.99) and snap (a GUESS 0.5 s spring). They are added by #8145 (uikit/paging.ts), which is not in this branch's merge order. Stack on #8145, or drop the reference and this leaf until it lands.
| * `setContentOffset(_:animated:)`'s curve (`paging.animatedScroll`: | ||
| * 0.3 s, sinusoidal), starting in the same frame. | ||
| * @source measured: UIKit lab, iOS 26.2 simulator, `.date` `.inline`, | ||
| * a tap on Next: the title changed in the first frame after the | ||
| * lift and the offset ran 960 → 1280 on the 0.3 s sinusoid from that | ||
| * frame (9.67, 21.33, 80, 160, 240, 298.67, 317.67 pt at 33, 50, 100, | ||
| * 150, 200, 250, 284 ms), Oct 23 2026 | ||
| */ | ||
| step: 'animatedScroll', |
There was a problem hiding this comment.
Same here: paging.animatedScroll, the 0.3 s sinusoid this leaf names, only exists in #8145, so step: 'animatedScroll' resolves to nothing on this branch.
| * In the first or last month `minimumDate` and `maximumDate` allow, | ||
| * the chevron toward the outside is disabled, not hidden: it stays in | ||
| * place, a touch on it goes to the header behind it, and its glyph | ||
| * takes the secondary label colour. |
There was a problem hiding this comment.
The measured tint below, rgba(0.235, 0.235, 0.263, 0.298) (#C5C5C7 on white), is UIKit's light tertiaryLabel (60, 60, 67 at 30 %); secondaryLabel is the same colour at 60 % (#8A8A8E on white). Suggest "takes the tertiary label colour".
There was a problem hiding this comment.
Fixed in ef989eb: thanks, #C5C5C7 is tertiaryLabel (60, 60, 67 at 30 %); the leaf now says the glyph takes the tertiary label colour.
7931cc3 to
8528591
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides on spec:AST-094. The new datePicker table rests on its draft FR1 and FR2, and registerMotionTable('datePicker', …) makes seven of its values theme-retunable motion roles, which the same record's FR4 and DEC-1 rule out. An engineering owner records an approval by commenting /approve-spec 8528591da149de284d216d27a605213e0a51f48d on this pull request, with FR4 and DEC-1 amended if themes may retune touch motion, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch carries its unmerged stack (#8043-#8045, #8052-#8076, #8062): 28,896 authored lines for a 454-line change, including
[feat]changes to the adaptive presentation and the theme API, so land those first or rebase so the diff is this commit alone. - The
platformadaptation condition (themeAdaptations.ts:82) adds an axis to the setspec:AST-012FR2 keeps closed, without amending that record. uikit.datePickeris exported from@astryxdesign/core/motion/experimental, so its changeset needs[experimental]instead of[chore] Internal.astryx docs motionsays components play these roles, but no component reads them yet, anddatePicker.tsstill points atpagingandpickertables that only #8145 and #8156 add.- The description needs the
## Writing rubric resultsthat a pull request creating a record must report.
[Automated review]
|
|
||
| // Each spring, duration and curve above is a motion role a theme may retune | ||
| // (motion/roles.ts). | ||
| registerMotionTable('datePicker', datePicker); |
There was a problem hiding this comment.
This call makes seven of the table's springs, durations and curves into motion roles that a theme can retune. The draft the table rests on says the opposite: spec:AST-094 FR4 ("Physics are not themable in touch presentations") and DEC-1 ("Rejected: tuning touch motion per theme"). Either an engineering owner amends FR4 and DEC-1, or this call and the seven astryx docs motion rows come out.
There was a problem hiding this comment.
Narrowed in ef989eb: only the three roles a component in this stack plays are registered (the month/year toggle's fade in #8206, the wheels' column turn in #8179 and #8209); the selection spring and the compact popover's springs are excluded until a component plays them. Whether themes may retune touch motion at all is the AST-094 FR4 / DEC-1 decision, which stays with that record's engineering owner.
| * 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 keeps the when vocabulary closed: "A when object may contain width, pointer, contrast, and motion … Future environment fields require explicit additions to this closed vocabulary." This adds platform without amending AST-012, so it needs an owner-approved amendment, which belongs in #8257 rather than on this branch.
There was a problem hiding this comment.
The platform condition comes from #8257 in this stack, not from this commit; its AST-012 amendment belongs to that pull request.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [chore] Internal: UIDatePicker's values measured on iOS 26.2 join the UIKit constants table. For the inline calendar they cover how a day tap selects, the animated selection's spring, the title following a drag, the month grid's release rule, the chevron's step and its retarget, the disabled chevron at a bound, and the month/year toggle's fades and chevron turn. For the wheels they cover the AM/PM column following the hour, the looping hour and minute columns, minutes that do not carry, the turn back onto a bound, the day clamped to the month and the minute interval. The compact popover's springs are included too. Each value comes with its source. Not exported from the package entry point; no component reads it yet. |
There was a problem hiding this comment.
@astryxdesign/core/motion/experimental exports the whole table as uikit (motion/experimental.ts:65), so uikit.datePicker ships on an experimental subpath. spec:AST-017 FR22 asks for an [experimental] changeset when an experimental surface gains something. Calling the table internal does not keep it private once it is reachable from a package path, so the changeset should also say where it is exported.
There was a problem hiding this comment.
Fixed in ef989eb: the changeset is now [experimental] and says the table is reachable as uikit.datePicker from @astryxdesign/core/motion/experimental.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
No component plays these values yet. Nothing outside packages/core/src/motion and theme reads a --motion-* variable or calls resolveMotion, and BottomSheet still settles on the theme's CSS transition. A theme author who follows this page, or sets one of the seven datePicker.* rows below, would see no change. Could this say that no component plays touch motion roles yet?
8528591 to
ef989eb
Compare
|
Replies to the automated reviews on ef989eb (the inline comments are answered in their threads):
|
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides on spec:AST-094: the new datePicker table rests on its draft FR1 and FR2, and registering three of its values as theme-retunable motion roles conflicts with the same draft’s FR4 and DEC-1. An engineering owner records that decision by commenting /approve-spec ef989ebc160e50325f7470c5d2adbb1b72063917 on this pull request (with FR4 and DEC-1 amended if themes may retune touch motion), and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch still carries its unmerged stack (29,043 authored lines for a 411-line commit), so land the predecessors first or rebase so the diff is this commit alone.
- The touch resolver’s own 744 px compact width (
utils/touchPresentation.ts:75-79) contradictsspec:AST-012FR9, which now sets compact at width belowmdor height below 500 through the theme’s exported definition. - The
platformadaptation condition (themeAdaptations.ts:82) extends the closed vocabulary ofspec:AST-012FR2 without an amendment. - Draft AST-094 FR8 still prescribes 0.5 s cross-fades and springs under Reduce Motion, which
design:reduced-motionDR1 and DR2 rule out. - The theme-roles changeset,
astryx docs motionandMEASUREMENTS.mddescribe roles and leaves that are gone or that nothing reads, and the description needs## Writing rubric resultsfor the record it creates.
[Automated review]
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
Current spec:AST-012 FR9 (on main since this branch was cut) makes compact "width below the effective md point (FR1) or … height below 500" for every resolver, "including the web touch presentation resolver … none keeps size numbers of its own", and DEC-6 rejects mapping compact to platform size classes. With 744 px here, a coarse-pointer screen 744-767 px wide opens the popover where FR9 promises the sheet, and a theme that overrides md cannot move the edge. After rebasing, could this build the query from the theme’s exported compactDefinition instead?
| * 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 still keeps when closed to width, height, pointer, contrast and motion ("Future environment fields require explicit additions to this closed vocabulary"), so platform needs an owner-approved FR2 amendment before it ships as stable theme API.
| - **FR8 — Reduce Motion substitutes, never jumps.** Under | ||
| `prefers-reduced-motion: reduce`, a touch presentation MUST behave as its | ||
| UIKit counterpart does with Reduce Motion and Prefer Cross-Fade Transitions | ||
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that | ||
| follows a finger, such as a throw, MUST keep its motion. **Proposed; pending | ||
| owner review:** this is the minimally moving form the draft `design:motion` | ||
| DR5 asks for, for touch presentations (owners: ernestt, cixzhang); the | ||
| alternative is UIKit's Reduce Motion alone, which keeps the slides of | ||
| navigation, sheets and modals. |
There was a problem hiding this comment.
Current design:reduced-motion (on main) now decides this: DR1 forbids springing content under reduced motion and DR2 limits a touch transition to "opacity only … 200 ms or less", with an immediate change on other pointers (DR3). The 0.5 s navigation cross-fade and the presentation and menu springs here contradict it, while the branch’s animator already follows the current record. Could FR8 defer to DR1-DR4?
There was a problem hiding this comment.
This branch no longer edits the AST-094 record; #8378 (on main) carries it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a value touch presentations play (`springs.sheetPresent`, `timing.reducedMotionCrossFadeMs`), at the root or in an adaptation rule. A role the theme leaves out keeps its measured default, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. Adaptation rules gain a `platform` condition, `ios` or `other`. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
timing.reducedMotionCrossFadeMs is no longer a role (uikit/timing.ts:158 excludes it, as design:reduced-motion DR4 requires), and unregistered ids pass defineTheme silently, so a theme author following this note gets no error and no effect. Another example role would keep the note true.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
At this head no component reads a --motion-* variable or calls resolveMotion, so none of the 48 listed roles changes anything yet, including the three datePicker.* rows. Could the page say no component plays touch motion roles yet, and could each role be registered in the pull request whose component plays it?
There was a problem hiding this comment.
The three datePicker.* rows are played in this stack: the column turn by TimeWheels (#8179) and MonthYearWheels (#8209), and the toggle's fade by #8206. Since 7ec94d0 nothing registers on import: registerDatePickerTable() runs from that touch code right before a role resolves. The page's general line about roles is #8076's.
| their own table (`uikit/datePicker.ts`); the month grid is a paging scroll view | ||
| and the wheels are UIPickerView's, so those parts use the paging and picker | ||
| values. | ||
|
|
||
| Done (UIKit lab, iOS 26.2 simulator, Oct 23 2026; in the date picker's table): | ||
| a day tap and `setDate(_:animated:)`, the title following the drag, the release | ||
| rule, the chevron step and its retarget, the chevron at a bound, the month/year |
There was a problem hiding this comment.
The release rule and the chevron step moved to #8200, and this branch has no picker table (scroll.paging holds two GUESS values), so this paragraph lists leaves the date picker’s table no longer holds.
There was a problem hiding this comment.
Fixed in 7ec94d0: the paragraph lists only the leaves this table holds, and names the release and arrow step as the pager's.
ef989eb to
4a19283
Compare
4a19283 to
d6646b4
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the governing spec for the three datePicker motion roles this pull request registers. spec:AST-094 FR1 and FR2 now cover the table's values, but FR4 and DEC-5 admit only the roles the record names or that a component's own record measures, and no component record measures these. An engineering owner records that decision by commenting /approve-spec 4d5fab8bbb1842c2bf8e9659674d9d33cabb7204 on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch still carries its unmerged stack (31,979 authored lines for a 417-line commit), so land the predecessors first or rebase so the diff is this commit alone.
- The motion role registry still uses preset names (
springs.interactive,timing.curves.easeInOutand 11 more) where currentspec:AST-094FR4 names roles by use (springs.fingerTracking,timing.curves.move), sodefineThemerefuses the record's names, and it offerssprings.keyboardandtiming.keyboardNotificationMsto themes although FR4 keeps keyboard motion out of them. defineThemealready accepts the threedatePickerids, so if they stay their changeset needs[feat]rather than[chore] Internal, and the claim that a role is offered only once a component plays it needs correcting.- The layout effect in
useGestureRecognizer.tsthat callssetEnabledshould name the system it synchronizes (architecture:react-component-runtimeINV3).
[Automated review]
| [chore] Internal: UIDatePicker's values measured on iOS 26.2 join the UIKit constants table. For the inline calendar they cover how a day tap selects, the animated selection's spring, the title following a drag, the chevron's retarget, the disabled chevron at a bound, and the month/year toggle's fades and chevron turn. For the wheels they cover the AM/PM column following the hour, the looping hour and minute columns (a short minute list included), minutes that do not carry, the turn back onto a bound, the day clamped to the month and the minute interval. The compact popover's springs are included too. Each value comes with its source. Not exported from the package entry point. | ||
|
|
||
| The motion the date and time inputs play from it becomes motion roles a theme may retune once the inputs' touch code registers the table (nothing registers on import), and `astryx docs motion` lists them: `datePicker.calendar.monthYearToggle.fadeMs` (the month/year toggle's fades) and `datePicker.wheels.columnTurn.durationMs` and `datePicker.wheels.columnTurn.curve` (a wheel column the picker turns itself). |
There was a problem hiding this comment.
defineTheme checks role ids against roleKinds.generated.ts, which the generator builds by calling registerDatePickerTable() itself, so the three datePicker.* ids are accepted at this head, not "once the inputs' touch code registers the table". Reachable from @astryxdesign/core/theme, they are public under architecture:public-component-api INV12, and spec:AST-017 FR7 gives a compatible capability [feat]. If the roles stay after the owner's ruling, could this changeset say [feat] and describe them as accepted now?
| * SwiftUI `interactiveSpring()` defaults: for a value that tracks a finger. | ||
| * @source apple-docs https://developer.apple.com/documentation/swiftui/animation/interactivespring(response:dampingfraction:blendduration:) | ||
| */ | ||
| interactive: {response: 0.15, dampingRatio: 0.86}, |
There was a problem hiding this comment.
Current spec:AST-094 FR4 names this role springs.fingerTracking ("a theme motion role named for its use"). Twelve more registered roles have the same mismatch: bouncy, smooth, snappy, uikitDefault, uikitSpringDuration, swiftUIDefault and swiftUISpring, plus timing.curves.caDefault, easeIn, easeOut, easeInOut and timing.implicitAnimationMs. springs.subMenuOpen and springs.subMenuBack are missing, so defineTheme throws on the record's names. Could the keys move to FR4's use names before the theme fields ship?
| * (motion/roles.ts). The rest waits for a component to play it, so a theme | ||
| * author is never offered a value that changes nothing. Nothing registers on |
There was a problem hiding this comment.
At this head a theme author is offered these three values: defineTheme accepts them and astryx docs motion lists them, while no runtime code calls this function. The generator calls every register<Name>Table export to build the list defineTheme checks. Either keep the ids out of the generated list until the touch code that plays them lands, or reword this comment.
| * iPhone 17 Pro, CAAnimation read on the keyboard host and a | ||
| * keyboardLayoutGuide-pinned view, Oct 23 2026 | ||
| */ | ||
| keyboard: {mass: 1, stiffness: 555.0265, damping: 47.118}, |
There was a problem hiding this comment.
Current spec:AST-094 FR4: "Motion that must stay in step with the on-screen keyboard is not a theme role: it rides the keyboard's own motion." registerSpringsTable() still makes this a role, so defineTheme accepts springs.keyboard, and the same goes for timing.keyboardNotificationMs. Could both join the table's exclude list, as press.scrollBrakeMs does in registerTimingTable()?
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the governing spec for the three datePicker motion roles this pull request registers: spec:AST-094 FR1 and FR2 now cover the table’s values and the role registry matches FR4’s named roles, but FR4 and DEC-5 admit only the roles the record names or that a component’s own record measures, and no date or time input record measures these. An engineering owner records that decision by commenting /approve-spec 9c82fdf6bc6718b94bfdc99b6b4765d8ad3f99c0 on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch still carries its unmerged stack (29,998 authored lines for a 416-line commit), so land the predecessors first or rebase so the diff is this commit alone.
defineThemealready accepts the threedatePickerids, so if they stay their changeset needs[feat]rather than[chore] Internaland the comment and description saying a role is offered only once a component plays it need correcting; if they wait, keep them out of the generated role list.
[Automated review]
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the governing spec for the three datePicker motion roles this pull request registers: spec:AST-094 FR1 and FR2 cover the table’s values, but FR4 and DEC-5 admit only the roles the record names or that a component’s own record measures, and no date or time input record measures these. An engineering owner records that decision by commenting /approve-spec 669d7076b171eb78fa19bb880f4b785d3bdfb2fc on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch still carries its unmerged stack (28,277 authored lines for a 416-line commit), so land the predecessors first or rebase so the diff is this commit alone.
pnpm lintfails onpackages/core/src/motion/useAnimator.test.tsx:80, where a test defines itsProbecomponent inside the test body (@eslint-react/no-nested-component-definitions).defineThemealready accepts the threedatePickerids, so if they stay their changeset needs[feat]rather than[chore] Internaland the comments saying a role is offered only once a component plays it need correcting; if they wait, keep them out of the generated role list.
[Automated review]
| vi.useFakeTimers(); | ||
| try { | ||
| const box: {current: Animator | null} = {current: null}; | ||
| function Probe() { |
There was a problem hiding this comment.
pnpm lint fails on this line at this head: @eslint-react/no-nested-component-definitions reports “Do not nest component definitions inside other components or props. Move it to the top level” (the only error among 133 problems), so the Lint workflow is red. Moving Probe to the top level, with box passed in as a prop, should clear it; the test comes from the stack, so the fix belongs in the pull request that owns it.
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the governing spec for the three datePicker motion roles this pull request registers: spec:AST-094 FR1 and FR2 cover the table's values, but FR4 and DEC-5 admit only the roles the record names or that a component's own record measures, and no date or time input record measures these. An engineering owner records that decision by commenting /approve-spec d9eac960b3b0501a59250d992032311e93d1e74f on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes before merge:
- The branch still carries its unmerged stack (30,120 authored lines for a 417-line commit), so land the predecessors first or rebase so the diff is this commit alone.
defineThemealready accepts the threedatePickerids, so if they stay their changeset needs[feat]rather than[chore] Internaland the comments saying a role is offered only once a component plays it need correcting; if they wait, keep them out of the generated role list.
[Automated review]
There was a problem hiding this comment.
Changes are needed before this can merge: the date picker values are well sourced and match spec:AST-094 FR1 and FR2, but two things need fixing.
- The branch still carries its unmerged stack (22,523 authored lines for a 344-line commit), so land the predecessors first or rebase so the diff is this commit alone.
datePicker.ts(lines 32-34) and the description say the touch surfaces read these springs, durations and curves directly and cite FR4 for none being a motion role, and the changeset repeats that they are not theme roles, but FR4 requires every touch spring, duration and curve a presentation plays to be a theme motion role and forbids playing a literal, so please say instead that the seven motion values join as roles named for their use once the date and time inputs' own record measures them (FR4, DEC-5).
[Automated review]
| * Every value here is an internal constant the touch surfaces read directly: | ||
| * none is a motion role (`spec:AST-094` FR4), so nothing here is a theme's to | ||
| * retune. |
There was a problem hiding this comment.
Current spec:AST-094 FR4 says the opposite for this table's springs, durations and curve: "Every spring, duration and curve a touch presentation plays MUST be a theme motion role named for its use … A component MUST read touch motion through these roles and MUST NOT play a literal." Could this say that the seven motion leaves become roles named for their use when the date and time inputs' own record measures them (FR4, DEC-5), that touch code plays them through those roles, and that only the rules stay internal constants? The description says the same, and the changeset calls the values internal constants, not theme roles.
There was a problem hiding this comment.
Reworded in f4655e3. The header now names the seven motion leaves: the selection spring, the toggle's fade, the compact popover's two springs and its label dim, and the column turn's duration and curve. It says they become motion roles named for their use, which touch code then plays, once the date and time inputs' own record measures them (spec:AST-094 FR4, DEC-5), and that those roles stay internal until the public motion-token set is decided (DEC-11). The rules stay internal constants. The changeset no longer calls the values not theme roles, and the description says the same.
There was a problem hiding this comment.
Changes are needed before this can merge: the date picker values are well sourced and match spec:AST-094 FR1 and FR2, but two things need fixing.
- The branch still carries #8076's stack (20,456 authored lines for a 347-line change), and the date picker commit needs none of that code, so rebase it onto
mainor land the predecessors first so the diff is this change alone. datePicker.ts(lines 38-39) and the description say the touch surfaces read the seven springs, durations and curves straight from the table until the date and time inputs' record measures them, but FR4 says a component reads touch motion only through roles and never plays a literal, so please say instead that no surface plays them before they are roles.
[Automated review]
| * motion-token set is decided (DEC-11). Until then the touch surfaces read | ||
| * them here. The rest are rules and stay internal constants. |
There was a problem hiding this comment.
Current spec:AST-094 FR4: "A component MUST read touch motion through these roles and MUST NOT play a literal." Until the date and time inputs' own record measures these seven values (DEC-5) there is no role to play them through, so a surface that reads them here to play them plays the literal FR4's verification fails. Could this say instead that no touch surface plays them before they are roles (the 16 rules can still be read here directly), with the description's "the touch code reads the values directly" changed to match?
There was a problem hiding this comment.
Reworded in c38d4fc. A leaf equal to a motion role FR4 names plays through that role: the compact popover's springs are springs.menuOpen and springs.menuClose, and the toggle's fade and its chevron's turn run timing.stateChangeMs on timing.curves.move. #8206's toggle drops its literal 250 ms for that role at its next push. The column turn, the selection spring and the label dim match no FR4 role, so they stay internal measured constants like every table value that isn't a role, and become roles only if the date and time inputs' record names them (DEC-5). No surface plays a bare literal. The description says the same.
The date picker domain of the UIKit constants table, each value sourced: the inline calendar's selection, the title following a drag, the chevrons' retarget and their state at a bound, the month/year toggle's fades and chevron turn and its wheels; the wheels' rules (the AM/PM column following the hour, looping columns, minutes without carry, the minute interval, the turn back onto a bound, the day clamped to the month); the compact popover's springs. Leaves equal to a motion role spec:AST-094 FR4 names play through that role: the compact popover's springs are springs.menuOpen and springs.menuClose, and the toggle's fade and chevron turn run timing.stateChangeMs on timing.curves.move. The leaves no FR4 role matches (the column turn, the selection spring, the label dim) are internal measured constants, like every table value that is not a role; they become roles only if the date and time inputs' record names them (DEC-5). Nothing is exported from the package entry point.
f4655e3 to
c38d4fc
Compare
github-actions
Bot
removed
the
needs:design-review
|
On scope: the branch now sits on main as one commit, the date picker's values, so the diff is this change alone. #8076 isn't needed for it. |
There was a problem hiding this comment.
This looks ready to merge: each new datePicker value names its source as spec:AST-094 FR1 and FR2 require, the table stays internal and registers no motion role, and the branch is now just this change. Two optional tidy-ups: pin datePicker.calendar.monthYearToggle.fadeMs to timing.stateChangeMs in values.test.ts, as the compact springs are pinned to the menu springs, and list the three new date picker checks in that file's @output header.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: every new datePicker value names its source as spec:AST-094 FR1 and FR2 require, the table stays internal to core, and it registers no motion role. Optional tidy-ups: pin datePicker.calendar.monthYearToggle.fadeMs to timing.stateChangeMs in values.test.ts, as the compact springs are pinned to the menu springs; name the minute wheel's step in meridiemFollowsHour's source, which goes from 12:58 PM to 1:02 PM on one hour turn; and list the three new checks in values.test.ts's @output header.
[Automated review]
|
|
||
| it('the compact date picker\u2019s popover opens and closes on the menu springs', () => { | ||
| expect(datePicker.compact.present).toEqual(springs.menuOpen); | ||
| expect(datePicker.compact.dismiss).toEqual(springs.menuClose); |
There was a problem hiding this comment.
Optional: the table's header says the month/year toggle plays timing.stateChangeMs, but nothing ties datePicker.calendar.monthYearToggle.fadeMs to it the way these lines tie the compact springs to the menu springs. One more assertion (with timing imported from '.') would keep a later re-measure of either from leaving two values for the toggle's fade:
expect(datePicker.calendar.monthYearToggle.fadeMs).toBe(timing.stateChangeMs);| * en_US: 11:58 AM, hour wheel +1 row → `valueChanged` 12:58 PM; +1 | ||
| * again → 1:02 PM, Oct 23 2026 |
There was a problem hiding this comment.
Optional: as written this capture goes from 12:58 PM to 1:02 PM on a single hour turn, which adds up only with the minute wheel's +4 rows from the minutesCarry capture in between. Naming that step would make the capture reproducible; the rule it supports (PM kept from 12 to 1) holds either way.
Sits on main: one commit, the date picker's values, plus merges from main. Review only this PR's change: vjeux/astryx@2c4f172...896dbbe
Maintainer impact
The DateInput, DateTimeInput and TimeInput touch surfaces need UIDatePicker's numbers and rules in the UIKit constants table (
spec:AST-094FR1, FR2). This adds the date picker's domain, each value sourced and internal to core. A leaf equal to a motion rolespec:AST-094FR4 names plays through that role; the others are internal measured constants (FR4, DEC-5).Intended invariant
Every leaf in
motion/uikit/has an allowed source; every guess names a measurement task (uikit.test.ts).Change
motion/uikit/datePicker.ts, measured on the iOS 26.2 simulator.Inline calendar
setDate(_:animated:)fades the circle on a critically damped 0.3 s spring.Compact popover: the menu springs; the label dims 0.47 s.
Wheels
minuteIntervaldivides 60 (≤ 30); a column past a bound turns back onto it; a missing day clamps to the month's end; year-and-month's month column doesn't loop.Roles and constants: the compact springs are
springs.menuOpen/springs.menuClose, and the toggle runstiming.stateChangeMsontiming.curves.move; surfaces play those roles. The column turn, selection spring and label dim match no FR4 role and stay internal measured constants, roles only if the date and time inputs' record names them (DEC-5). No surface plays a bare literal; the CLI's motion docs are unchanged.MEASUREMENTS.mdrecords the captures.Evidence
uikit.test.tschecks every new leaf's source.values.test.tschecks that the selection spring is critically damped with a 0.3 s response and settles in 0.441 s, as UIKit reported.Scope
Testing
pnpm exec vitest run packages/core/src/motion: all passed.packages/cli/test/experimental-isolation.test.mjspasses unchanged: the motion docs list no date picker roles.pnpm -F @astryxdesign/core typecheck,pnpm check:changesets, and ESLint and Prettier on touched files.pnpm exec vitest run packages/core/src/motion packages/cli/test/experimental-isolation.test.mjs: 13 files, 472 tests, including the short-minute-list value measured in the UIKit lab (Oct 24); the CI duration guard;pnpm buildthen the rootpnpm typecheck,check:changesets,check:knowledgeandCI=true pnpm lint.