Motion Lab: narrow the exit rule, mount real components, add an idle-cost criterion - #5613
Merged
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:code-review
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
ernestt
force-pushed
the
motion-exit-cost
branch
from
August 27, 2026 21:13
bf64105 to
3c39be4
Compare
github-actions
Bot
removed
the
needs:code-review
The lab inherited the brief's exit rule and treated it as settled. Checked against the references the brief itself cites, it is two rules with very different support, and only one of them holds. **Direction is unanimous.** Astryx's published page says "match the entrance"; Emil's guidance says "exit the way it entered". Whenever an exit is animated, this governs it. Unchanged, and now stated as the strongest claim in the proposal rather than a footnote to it. **"Shorter than the entrance" is not a law.** beUI states it plainly — "let old content leave faster than new content arrives" — but Emil argues asymmetric timing specifically around deliberate actions versus system responses, a distinction the brief drops. The `--duration-exit` / `--duration-enter` mapping encodes it universally, which is stronger than anything cited. Both tokens keep their values as defaults; the ratio stops being asserted as a rule. **"Every presence surface animates out" is contradicted by both sources.** Emil says high-frequency UI often should not animate its exit at all. The published page says the same thing about the same surfaces, naming tooltips, hover cards and dropdown menus. Criterion 7 was failing components for obeying the only guidance anyone had written down. The corrected rule, which is now the lab's position: animate an exit only when it aids orientation; when animated, keep its direction spatially consistent with the entrance; timing may be shorter when the dismissal is a system response, chosen by purpose and frequency rather than applied universally. ## What changed - Criterion 7 narrowed from "presence surfaces animate both directions" to orientation-gated, and re-tagged `overreach` rather than `reversal`. - **A contradiction inside the rubric surfaces and resolves.** Criterion 2 says anything seen 100+ times a day gets no motion; criterion 7 made an instant dismissal a blocker for overlays. Tooltip is both, so the two could not both be satisfied. Frequency now decides whether an exit is warranted, and criterion 7 governs only the exits that are. Both criteria carry the cross-reference. - The exit-gap page sorts the eleven components by whether an exit aids orientation: four earn one (Dialog/AlertDialog, Lightbox, BottomSheet and MobileNav, with CommandPalette flagged as the row most likely to flip on frequency), seven do not. Three of the seven are quoted verbatim in the published page, so those are not a judgement call. - That inverts the page's own demos, and it now says so: all four surfaces it animates are in the "instant is fine" column. They stay, because you cannot judge whether a dismissal aids orientation without seeing the alternative, but they read as the question rather than the fix. - A new `SOURCES` ledger on the published page shows the working — each claim, what each source actually says, and the verdict. - A second conflict entry splits the timing claim from the direction claim, since they have different evidence and different resolutions. - New `overreach` severity throughout: the proposal claims more than its sources support, which is a different problem from two documents disagreeing and has a different fix — narrow the proposal, do not rewrite the page. The infrastructure finding survives intact and is the reason to still do the work: `useLayer` cannot express an exit at all, which blocks the four surfaces that want one. Same change, same size. The justification and the rubric gate that follows from it are what were wrong. Test plan: - `pnpm -F @astryxdesign/sandbox build`: passes, all fifteen routes prerender. - Exported HTML asserts the reframing landed: triage renders 4 earns / 7 instant with 3 named-in-docs tags, the corrected rule appears on the exit-gap page, the source ledger renders both references on the published page, and the rubric shows the criterion 2/7 cross-reference. - Old blanket rule string is absent from the rubric output. - 315 internal links resolve, 0 broken. - `npx tsc --noEmit` and `npx eslint` on the lab: clean.
…st model
Two things, both about the lab making claims it can back.
## The comparisons now mount the real component
A before/after is worth nothing if the "before" is a drawing of the component
rather than the component. Three panes changed:
- The checkbox tick's "today" pane is real `CheckboxInput`.
- Both TabList panes are core's `TabList`. The proposed one adds a
`hasTravellingIndicator` prop, one indicator in the strip moved with
`transform`, and a context flag telling each Tab to suppress the indicator it
draws for itself.
So the difference on screen is the diff. Forks live in `proposed/`.
Building it turned up things a mock-up would have hidden. The travelling
indicator has to be measured from the DOM, because tabs size themselves —
`layout="fill"`, a font swap or a resize all move the target without anything
in React changing — so the proposal carries a ResizeObserver, and that is now
visible as the thing to review. The indicator also needed the strip's bleed
padding accounted for, and core's dev-only "tablist owns only tabs" warning had
to learn that an `aria-hidden` child is not owned.
**The SideNav rail demo stays hand-built, deliberately.** A fork of core's
`SideNav` needs `SideNavCollapseContext` on the public surface — `SideNav` is a
*provider*, and core exports only the reader hook `useSideNavCollapse`. The
one-line fix is `export {SideNavCollapseContext}` beside the hook, exactly how
`SideNavRenderContext` is already exported three lines below it, but that is a
public API surface change and wants its own review, so it is deferred and this
PR touches no package code.
Worth recording, because the workaround is a trap: importing the context by
relative path compiles and is silently wrong. The app resolves
`@astryxdesign/core/*` to `dist` while a relative import reaches `src`, so the
fork provides one React context object and core's `SideNavItem`,
`SideNavHeading` and `SideNavSection` read another, see the default
`{isCollapsed: false}`, and render labels inside a narrow rail with no error
anywhere. The rail demo is weaker than the TabList one until the export lands,
and both the code and the page say so.
`astryx swizzle` emitted code that did not typecheck for both components:
`NavItem`, `useMenuHover` and `SideNavCollapseContext` for SideNav,
`isRtlElement` and `MENU_ITEM_SELECTOR` for TabList. All are internals a
swizzled component genuinely needs. Recorded at each import and on the previews
page.
## Criterion 13: idle cost
DESIGN.md's hard rule is that aesthetics never outrank performance, and the
rubric had no criterion for the one case where a motion decision keeps costing
after the interaction ends. A one-shot transition stops. A loop runs every
frame, on every page, for as long as the page is open — including while the
user does nothing. kt-fr88 is the reference case: an ambient colour fade
repainting the full document at 60fps on an idle page, 1,591 paints in 28s,
GPU 78% busy; D117309888 took a 4s idle window from 1,414 paints to 93.
`motionCost.ts` carries the rule, the four tiers, and every loop in core, lab
and this lab measured against it. The finding is not what I expected:
- **Core is the clean part.** All five of its loops animate `opacity` or
`transform`, and all five have a reduced-motion arm. The property discipline
is already there.
- **Both high-cost loops are in lab.** `ChatReasoning` animates
`background-position` with no reduced-motion arm at all — it fails every axis
of the rule at once, for 4s per iteration, forever, behind streaming text.
`CircularProgress` animates SVG stroke geometry, and has no row in the
preview plan to fix it.
- **The systemic gap is containment.** Of the nine loops the packages publish,
exactly one takes a deliberate layer (Spinner) and none are paint-contained.
That costs least on a lone spinner and most on `Skeleton` and `StatusDot`,
the two that multiply — a loading table is dozens of uncontained loops.
Every one of the 60 preview-plan rows now carries an idle-cost label: 55 idle,
1 low, 3 medium, 1 high, with a reason on each that is not idle.
**This lab was the worst offender.** Three of its five loop demos animate a
paint-only or layout property — a `box-shadow` pulse, a `background-position`
shimmer, and a bar on `inset-inline-start`. They are kept, because they are the
demonstration of what the criterion forbids, but they are now `contain: paint`
so the demonstration cannot repaint the page around it. Two transform loops take
a layer gated on `prefers-reduced-motion: no-preference`.
Found while containing them: the stylesheet's `.loop` rule, which was supposed
to stop loops under the reduced-motion policies, matched nothing — no element
carries the class, and LoopRig sets `animation: none` inline, which would
outrank it anyway. Same trap as the duration aliases. Replaced with a comment
saying so, rather than a rule that looks load-bearing and is not.
Test plan:
- `pnpm -F @astryxdesign/sandbox build`: passes, all fifteen routes prerender.
- Exported HTML asserts the real components: 2 `astryx-tab-list` instances on
the previews page, real checkbox markup, and 15 idle-cost badges plus the
high-cost reason on the plan page.
- `git status packages/` is empty: no package code is touched.
- `npx tsc --noEmit` and `npx eslint` on the lab and the touched core file:
clean.
- 315 internal links resolve, 0 broken.
- Loop inventory read off source, not assumed: every keyframe set in core, lab
and this lab was opened and its animated property recorded.
…applies Every internal link in the Motion Lab pointed off the deployed site. On the PR preview they resolved to `facebook.github.io/pages/motion-lab/...` instead of `facebook.github.io/astryx/pr/<n>/sandbox/pages/motion-lab/...`, which is the org's root site rather than the sandbox. The sandbox deploys under a basePath and `next/link` prepends it automatically, which is why the rest of the app — SandboxNav, ProjectCard — has always linked correctly. This section used Astryx's own `Link`, which renders a plain anchor, so a hand-written root-absolute href escaped the prefix entirely. Astryx already has the seam for this: `Link` and `ListItem` both resolve their element through `useLinkComponent()`, and `LinkProvider` sets it for a subtree. One provider in the section layout, pointing at `next/link`, fixes all 18 link sites and the nav panel at once. No per-link changes, and nothing to remember when adding a page. Worth recording the wrong turn, because it looked right: I first assumed `ListItem` did NOT read the provider and prefixed its href by hand. That double-prefixed every nav item — `/astryx/pr/5613/sandbox/astryx/pr/5613/ sandbox/...` — and 404'd. `ListItem` does route through the provider. Nothing in this section should apply the basePath itself; Next does it exactly once. Both files now say so. Also fixed the verification that missed this. The link check compared hrefs against the built tree but ran on a build with an empty `SANDBOX_BASE_PATH`, where a root-absolute href is correct by construction — so it passed while the deployed preview was broken. It now builds with the real PR basePath and asserts two things separately: that every internal href carries the prefix, and that it resolves to a file. That is what caught the double-prefix. Test plan: - `SANDBOX_BASE_PATH=/astryx/pr/5613/sandbox pnpm -F @astryxdesign/sandbox build`: passes, all fifteen routes prerender. - 315 internal links: 0 escaping the basePath, 0 broken against the built tree. - `npx tsc --noEmit`: clean.
ernestt
force-pushed
the
motion-exit-cost
branch
from
August 27, 2026 21:56
3c39be4 to
c227338
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits on the Motion Lab, both about it making claims it can back.
1. The exit rule was stronger than its sources
The lab inherited the brief's exit rule and treated it as settled. Checked
against the references the brief itself cites, it is two rules with very
different support:
So criterion 7 was failing components for obeying the only guidance anyone had
written down. It is narrowed to: animate an exit only when it aids
orientation; when animated, keep its direction spatially consistent with the
entrance; timing may be shorter when the dismissal is a system response, chosen
by purpose and frequency rather than applied universally.
This surfaced a contradiction inside the rubric. Criterion 2 says anything
seen 100+ times a day gets no motion. Criterion 7 made an instant dismissal a
blocker for overlays. Tooltip is both, so the two could not both be satisfied.
Frequency now decides whether an exit is warranted; criterion 7 governs only the
exits that are. Both criteria carry the cross-reference.
The exit-gap page now sorts the eleven components by whether an exit aids
orientation: four earn one (Dialog/AlertDialog, Lightbox, BottomSheet and
MobileNav, with CommandPalette flagged as most likely to flip on frequency),
seven do not. Three of the seven are quoted verbatim in the published page, so
those are not a judgement call — which also means the page's own four demos are
all in the "instant is fine" column, and it now says so.
The infrastructure finding survives intact:
useLayercannot express an exit atall, which blocks the four surfaces that want one. Same change, same size. The
justification and the rubric gate were what were wrong.
2. Real components, and an idle-cost model
The comparisons now mount the real component. A before/after is worth
nothing if the "before" is a drawing.
CheckboxInput,SideNav(both panes)and
TabList(both panes) are now core's own, with the proposals applied toswizzled forks in
proposed/— so the difference on screen is the diff.Building them turned up what a mock-up hides: the travelling indicator has to be
measured from the DOM, because tabs size themselves, so the proposal carries a
ResizeObserver that is now visible as the thing to review.
One additive export in core.
SideNavCollapseContextis now exported besideuseSideNavCollapse, asSideNavRenderContextalready was three lines below.Without it a swizzled SideNav cannot compile — it is a provider, and only the
reader hook was public. The workaround compiles and is silently wrong: the app
resolves to
distwhile a relative import reachessrc, yielding two distinctReact contexts and children that render expanded inside a narrow rail with no
error anywhere.
Criterion 13: idle cost. A one-shot transition stops; a loop runs every
frame, forever, including while the user does nothing. Reference case is
kt-fr88 — an ambient colour fade repainting the full document at 60fps on an
idle page, 1,591 paints in 28s, GPU 78% busy; D117309888 took a 4s idle window
from 1,414 paints to 93.
Every loop in core, lab and this lab was read off source, not inferred:
opacityortransform, andall five have a reduced-motion arm.
ChatReasoninganimatesbackground-positionwith no reduced-motion arm at all, for 4s per iteration,behind streaming text.
CircularProgressanimates SVG stroke geometry — andhas no preview-plan row to fix it.
exactly one takes a deliberate layer (Spinner) and none are paint-contained.
That costs most on
SkeletonandStatusDot, the two that multiply.All 60 preview-plan rows carry an idle-cost label (55 idle, 1 low, 3 medium,
1 high), with a reason on each that is not idle.
This lab was the worst offender — three of its five loop demos animate
paint-only or layout properties. They stay, because they demonstrate what the
criterion forbids, but they are now
contain: paintso the demonstration cannotrepaint the page around it.
Found while containing them: the stylesheet's
.loopreduced-motion rulematched nothing. No element carries the class, and LoopRig sets
animation: noneinline, which would outrank it anyway.Swizzle bugs worth their own issue
Nothing swizzled here compiled as emitted.
astryx swizzle SideNavneedsNavItem,useMenuHoverandSideNavCollapseContext;astryx swizzle TabListneeds
isRtlElementandMENU_ITEM_SELECTOR. Every one is an internal aswizzled component genuinely requires. Recorded at each import and on the
previews page.
Test plan
pnpm -F @astryxdesign/sandbox build: passes, all fifteen routes prerender.(A stale
.nextcache masks the new core export —rm -rf .nextif you see"SideNavCollapseContext is not exported".)
astryx-side-navnodes and 2astryx-tab-listinstances on the previews page, real checkbox markup, 15idle-cost badges and the high-cost reason on the plan page, the corrected exit
rule and the 4/7 triage on the exit-gap page, and the source ledger on the
published page.
npx tsc --noEmitandnpx eslinton the lab and the touched core file: clean.