Repository navigation
refactor(Toast): a moved toast's card runs no hidden transition - #10223
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Toast · Storybook | 88.9 kB (+71 B) · 9.6 kB CSS (−14 B) | 1,644 (+1) | — · — · — |
Package size: @astryxdesign/core 6.68 MB packed (+36 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 21 rows; run pnpm api:report.
Wide change (shared styles: packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
|
AI review status for this pull request.
|
|
/approve-spec c048c03 |
There was a problem hiding this comment.
Changes requested: the code change looks right, but the changeset describes a visible defect that main does not have.
.changeset/toast-moved-card-still.mdsays a moved toast "restarts its card's slide", but on main the moved card'stransformtransition runs from the identity to the identity (in Chromium its transform, opacity and position stay unchanged on every frame when a Dialog or a modal Drawer opens or closes over it, and the 0.7.9 notes already say a moved toast stays where it is), so please reword it to what changes, for example "A toast's card no longer runs a hiddentransformtransition, with itstransitionrunandtransitionendevents, when a Dialog, a modal Drawer or a scrim sheet opens or closes over it. The toast already stayed still, and nothing on screen changes."
Please make the description's User impact match that wording. The card's new entry gate, the private-property cleanup and the new Drawer case look good, and a toast's first entrance is unchanged.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A toast already on screen no longer restarts its card's slide when a Dialog, a modal Drawer or a scrim sheet opens or closes over it. Its card now enters only once, as its row does. |
There was a problem hiding this comment.
This describes a visible restart that main does not have. Since the 0.7.9 fix, a moved card's restarted transform transition runs from translateY(0px) to its resting translateY(0px) scale(1), the identity on every frame, so the card neither slides nor fades. In Chromium its transform, opacity and box stay unchanged through each Dialog and Drawer move. Suggested wording: "A toast's card no longer runs a hidden transform transition, with its transitionrun and transitionend events, when a Dialog, a modal Drawer or a scrim sheet opens or closes over it. The toast already stayed still, and nothing on screen changes."
There was a problem hiding this comment.
Reworded at d873097: the changeset now says the card no longer runs a hidden transform transition, with its events, and that the toast already stayed in place. The description says the same.
c048c03 to
d873097
Compare
|
/approve-spec d873097 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The required checks wait for a maintainer to add the ci:run label; CI runs once it is added.
The changeset and description now match the change: on main a moved toast's card runs an identity-to-identity transform transition, with its events, that moves nothing on screen, and this change removes it. The host-move browser spec fails its three already-shown cases on main and passes on this head, and a toast's first entrance is unchanged.
[Automated review]
A top-layer ToastViewport moves its host into the latest open modal and back out. Each move renders the toast's card again, and the card's starting style applied once more. So a toast already on screen started a transform transition at every move: a Dialog, a modal Drawer or a scrim sheet that opened or closed over it. The card's starting style now applies only until the toast's row starts to open, as the row's own does, and the row passes that state to the card. A move then renders an entered card at rest and starts nothing. This replaces the two private start properties the row set on the card, so they leave the docs and the registry's list too.
d873097 to
a3c7630
Compare
|
/approve-spec a3c7630 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is still a draft, and its required checks run once it is marked ready for review and a maintainer adds the ci:run label.
The [internal] changeset and the description match the change: on main a moved toast's card runs an identity-to-identity transform transition, with its events, that moves nothing on screen, and this change removes it. The host-move browser spec fails its three already-shown cases on main and passes on this head, and a toast's first entrance is unchanged.
[Automated review]
Maintainer impact
When a
Dialog, a modalDraweror a bottom sheet with a scrim opens or closes while a toast is on screen, the top-layerToastViewportmoves its host into the modal or back out, and each toast card that has already entered runs a hiddentransformtransition. Since #10086 it runs from the identity to the identity, so nothing moves on screen, but the card firestransitionrunandtransitionendat every move, and its entrance depends on two private custom properties that the row sets only so that the replayed starting values match the resting ones. AlertDialog's touch exit (#8084) moves a toast twice for each alert. Code and tests that watch a toast's transitions see one start per move that changes nothing.Intended invariant
component:ToastFR5: "A new card slides in from its edge and fades in". A toast's first entrance stays the same.component:ToastFR12: a top-layer viewport moves into the latest open modal and back, "keeping each toast, its timer, and focus inside the viewport".Change
Toastcard keeps its entrance.--_toast-start-opacityand--_toast-start-slide), so they also leave the docs and the derived-variable registry's list.The host still moves as a DOM node, and React sees nothing. Moving the toasts through a React portal instead would remount them: a portal whose container changes is a new portal to React, so every toast would enter again. A toast left outside an open modal cannot take a tap or focus, so it has to live inside the modal.
Evidence
transformtransition on its card: one when a Dialog closes, one when it opens, two when a Drawer closes and opens again. Chromium and WebKit show the same. On a Storybook built from main, the three cases for a toast already shown fail three runs out of three in each engine, each listingcard transform.Scope
Testing
Local gates at c048c03, on main add0516; this head changes only the changeset, now an
[internal]entry:Toast/__tests__/ToastViewport.hostMove.interaction.browser.spec.ts(it also records every transition or animation that starts on the toast's row or card, and a new case covers a modal Drawer): Chromium twice and pinned to 2 cores three times, 5 of 5 each; WebKit in CI's container five times, 25 of 25. The bottom sheet's and the drawer's toast outlet accessibility contracts (6) and the Toast, Layer, Dialog, Drawer, BottomSheet and theme units (3,050) pass.CI=true pnpm lint(no errors), the build,pnpm typecheck, Storybook,pnpm test:ui(21,545),pnpm test:node(16,817),check:knowledge,check:repo,specs:for --diffandreview:prepass.api:report --checkreports drift already on main (the CLI'sComponentDeprecationand search types), none of it in Toast.