Repository navigation
feat(core): one progress module for the motion foundation - #9755
Conversation
progressCurve, mapProgress, mapRange and presentedProgressVelocity in motion/progress.ts, internal, one velocity unit (progress per second). Rebuilt on main from the pull request's own change, without the open dependency chain it no longer needs. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
|
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
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the module looks correct, and its 12 tests, tsc and ESLint pass, but it adds a second copy of math the motion folder already has.
Blocking
progressCurve(progress.ts:56–70) duplicates the privatespringProgressinanimatedLayout.ts:144–151(same solve, same 0.001 settle epsilon, same exact landing, identical output), so please buildspringProgressonprogressCurveor replace it, leaving one implementation.
Non-blocking
- The file header (
progress.ts:3–39) runs well past the roughly 12-line header limit and repeats themapProgressandpresentedProgressVelocityJSDoc; trimming it to@file,@input,@output,@positionandSYNC:would fix both. - The
STEPcomment (progress.ts:49) restates the name, andmapProgresssteps the driver's own units rather than a progress, so a clearer name such asFINITE_DIFFERENCE_STEPcan replace it.
[Automated review]
| epsilon = SETTLING_EPSILON, | ||
| }: {velocity?: number; epsilon?: number} = {}, | ||
| ): MotionCurve { | ||
| const curve = solveSpring(toSpring(spring), {from: 0, to: 1, velocity}); |
There was a problem hiding this comment.
This is the same construction as the private springProgress in packages/core/src/motion/animatedLayout.ts:144–151 (unit move, settleTime(0.001), exact landing); for the settle spring and three others the two agree bit for bit. Please build springProgress on progressCurve (durationMs = Math.max(1, progress.duration * 1000), remaining = t => 1 - progress.at(t / 1000)) or replace it, so one implementation remains.
| type SpringDescription, | ||
| } from './spring'; | ||
|
|
||
| /** The step of the finite differences taken on a progress. */ |
There was a problem hiding this comment.
Non-blocking: the comment restates the name, and mapProgress takes this step on the driver's own value (px for a sheet's y), not on a progress. Suggest renaming to FINITE_DIFFERENCE_STEP and dropping the comment.
| * An interrupted motion continues from what is on screen: the | ||
| * block reads the presented values and velocities from the animator at one | ||
| * `now` (`get`, `getVelocity`), restarts a path from those values, and starts | ||
| * its progress with the presented velocity projected on the path's tangent | ||
| * at its start, v·t / (t·t), where t = d(values)/dp. Channels are weighted | ||
| * into one unit first (px per unit of scale, say), so a scale and a | ||
| * translation count alike. | ||
| * | ||
| * A companion follows a driving curve through `mapProgress`, most often | ||
| * with `mapRange`: `mapProgress(sheetY, mapRange([closedY, openY], [0, dim]))` | ||
| * dims a scrim as a sheet rises. Played on another animator, it stays in | ||
| * lockstep only when it starts with its driver: pass the driver's start, | ||
| * `scrim.play({opacity}, {at: start})` with `start = await handle.started` | ||
| * (or one shared `at` for both calls); two calls from rest otherwise each | ||
| * start at their own commit. |
There was a problem hiding this comment.
Non-blocking: these paragraphs take the header to 37 lines (the convention is about 12) and repeat the JSDoc of presentedProgressVelocity and mapProgress. Suggest keeping @file, @input, @output, @position and SYNC:, and moving the lockstep sentence into the mapProgress JSDoc.
…ressCurve CodeBunny on #9755: springProgress solved the same spring as progressCurve. It now reads progressCurve (remaining = 1 - progress), so one implementation remains. Same curve, same settle epsilon (0.001) and duration.
There was a problem hiding this comment.
Recommending approval: springProgress in animatedLayout.ts now reads progressCurve, so one spring-progress implementation remains, and the glide's curve is unchanged (same duration, every frame within 1.1e-16 of the move).
Non-blocking
- The
progress.tsfile header (lines 3–39) still runs well past the roughly 12-line header limit and repeats themapProgressandpresentedProgressVelocityJSDoc; trimming it to@file,@input,@output,@positionandSYNC:would fix both. - The
STEPcomment (progress.ts:49) restates the name and describes a step on a progress, whilemapProgresssteps the driver's own units; a name such asFINITE_DIFFERENCE_STEPcan replace it.
[Automated review]
cixzhang
added
the
ci:run
Supersedes #8497 by @vjeux, which stays the author of this commit. #8497's branch carries the whole open chain under #8360, about 27,000 lines against
main. This module needs nothing from that chain: everything it imports (Animator,MotionCurveandMotionPropertyfrommotion/animator;solveSpring,toSpring,SETTLING_EPSILONandSpringDescriptionfrommotion/spring) is already onmain. So this pull request is #8497's own change alone, rebuilt onmain.Maintainer impact
Motions that move several properties along one path (a popover growing out of its button while it fades in, a scrim's dim following a sheet) all need the same four pieces of math. Without one module, each presentation keeps its own copy. Several open motion pull requests build on this one.
Intended invariant
No behavior change. The module is internal, and no component imports it yet. It follows
spec:AST-094FR3 and FR6, with one velocity unit throughout: progress per second.Change
A new internal module,
motion/progress.ts, with its unit test, a README row and a[chore]changeset.animatedLayout's privatespringProgressnow readsprogressCurveinstead of solving the same spring itself, so one implementation remains (same curve, same 0.001 settle epsilon and duration):progressCurve(spring, {velocity, epsilon}): a spring's progress from 0 to 1 as a closed-form curve, exactly 1 once it settles;mapProgress(driver, f): a curve that follows another throughf;mapRange(input, output, {clamp}): the clamped linear map most companions use;presentedProgressVelocity: the progress velocity that continues what an animator shows.Evidence
main; feat(core): one progress module for the motion foundation #8497 can't merge as it stands (it carries an open chain).tscis clean against currentmain, and the 12 progress tests pass.animatedLayout's settle curve is the same spring, read as 1 − progress; the motion, List and TreeList suites pass (1,042 tests).Scope
Testing
pnpm exec vitest run packages/core/src/motion/progress.test.ts: 12 passed.useAnimatedLayout, List and TreeList suites: 1,042 passed.pnpm exec tsc -p packages/core --noEmit: clean.pnpm check:knowledge,pnpm check:changesets: pass.pnpm review:pre: 0 findings; 335 authored lines.Co-authored-by: Cindy Zhang (rebuilt on
mainfor the motion foundation stack).