Repository navigation
docs(core): the role test's header and the motion README's files name the internal roles - #8502
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
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: 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.
Changes are needed for one line: the corrected roles.test.ts header and the new roles.ts row match spec:AST-094 FR4 and the code, but the new roleValue.ts row in packages/core/src/motion/README.md says that file reads a role's CSS form back, while roleValue.ts has no parser (its header says “no registry and no parsing”) and parseMotionRoleValue lives in roles.ts. Moving “read back” to the roles.ts row would make the Files table accurate. Optionally, while the header is open: its @input line still says the shared tables are “registered on import”, though each table's register function now says nothing registers on import, which the approving review on #8257 also pointed out.
[Automated review]
| | `roles.ts` | The internal motion roles: the springs, durations and curves `spec:AST-094` FR4 names, registered by the shared tables. | | ||
| | `roleValue.ts` | A role's CSS form, written and read back: a spring's duration and bounce, a duration in ms, a curve's `cubic-bezier()`. | |
There was a problem hiding this comment.
roleValue.ts only writes a role's CSS form: its header says “no registry and no parsing”, and the read-back is parseMotionRoleValue in roles.ts. Suggest moving “read back” to the roles.ts row, for example roles.ts: “The internal motion roles: the springs, durations and curves spec:AST-094 FR4 names, registered by the shared tables; reads a role's CSS value back.” and roleValue.ts: “A role's id, CSS variable and CSS form: a spring's duration and bounce, a duration in ms, a curve's cubic-bezier().”
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
This looks ready to merge: the corrected roles.test.ts header and the two new rows in the motion README's Files table match the 17 roles spec:AST-094 FR4 names and the code they describe, and the [chore] changeset covers the README the core package ships. The earlier requested fix is in, with the read-back now described in the roles.ts row. Nothing further is needed from you.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: the corrected roles.test.ts header and the two new rows in the motion README's Files table match the 17 roles spec:AST-094 FR4 names and the code they describe, and the [chore] changeset covers the README the core package ships. The merge from main kept its README rows intact, and nothing further is needed from you.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: the corrected roles.test.ts header and the two new rows in the motion README's Files table match the 17 roles spec:AST-094 FR4 names and the code they describe, and the [chore] changeset covers the README the core package ships. The latest merge from main kept main's README rows intact. Optionally, refresh the description's Correction bullet, which still says roleValue.ts reads a role's CSS form back; the README itself rightly puts that read-back on roles.ts.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: the corrected roles.test.ts header and the two new rows in the motion README's Files table match the 17 roles spec:AST-094 FR4 names and the code they describe, and the [chore] changeset covers the README the core package ships. Optionally, refresh the description's Correction bullet, which still says roleValue.ts reads a role's CSS form back; the README rightly puts that read-back on roles.ts. Two notes from the #8257 review this follows remain for a later change: the comment at roles.test.ts lines 33-34 still says the tables are registered "the way the generator does", though no generator exists yet, and the springs.ts header still says it takes no input ("Nothing; data only") although the file now imports registerMotionTable.
[Automated review]
… the internal roles roles.test.ts's header says what it proves now: the registered roles are exactly the ones spec:AST-094 FR4 names, every other table value is no role, and a role's CSS value reads back as the same motion, with the tables registered by the test. The motion README's files table describes roles.ts that way and adds roleValue.ts. Docs and a test header only.
7e1e67d to
9667b53
Compare
There was a problem hiding this comment.
This looks ready to merge once it leaves draft: the corrected roles.test.ts header and the motion README's roles.ts and roleValue.ts rows match the 17 roles spec:AST-094 FR4 lists and the code they describe, and the [chore] changeset covers the README the core package publishes. Nothing further is needed from you. Optionally, a later change could refresh two older notes outside this diff: the "the way the generator does" comment at roles.test.ts lines 33-34, and the "Nothing; data only" @input in the uikit/springs.ts and uikit/timing.ts headers.
[Automated review]
cixzhang
added
the
ci:run
One commit on
main: vjeux@9667b538b5.Why
Someone reading the motion code to learn which values are motion roles gets a wrong answer from two files: the roles test's header says every spring, duration and curve of a registered table is a role, and the motion README's file table leaves out the two files that hold the roles.
What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion). #8257 made only the values
spec:AST-094FR4 names into roles (registerMotionTable(id, table, {roles})), kept inroles.tswith their CSS form inroleValue.ts. Its review's item 3 asked for these two descriptions to follow.Options and tradeoffs
roles.ts.Why this choice
The code already holds the list. The header and the README only need to say what it is and where it lives.
How to check it
packages/core/src/motion/roles.test.ts: its header says the registered roles are exactly the ones FR4 names, each by its table path, and that a component's own measured spring is no role.packages/core/src/motion/README.md: the files table listsroles.tsandroleValue.ts.pnpm exec vitest run packages/core/src/motion/roles.test.ts: it passes as before; only comments and the README changed.Evidence
pnpm test:ui(17,016), the node suites, lint, the knowledge check, and the changeset checks; Prettier reports every changed file clean.[chore]changeset keeps the release coverage check green.Scope
[chore]changeset for the coverage check).