feat(Modal,Backdrop): Add opt-in animation support#12552
Conversation
[WIP] This is an initial and in-progress implementation of Modal and Backdrop animation support. These changes were cherry-picked (file-level) from branch gmurcia/tearsheet, originating from the following commits: - c027e56 chore: Initial modal/backdrop animation impl - 1927470 chore: Animation tweaks. - a7f4102 chore: Implement TearsheetGroup for unbounded tearsheet levels Commit-generated-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…AnimationsProvider + hooks Replace custom `animated` prop with `hasAnimations` across Modal, ModalContent, ModalBox, and Backdrop. Wire up `useHasAnimations` hook in ModalContent and Backdrop so both participate in AnimationsProvider context. Add second example demonstrating the AnimationsProvider path. Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
WalkthroughChangesModal and Backdrop animation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Modal
participant ModalContent
participant ModalBox
participant Backdrop
User->>Modal: toggle isOpen
Modal->>ModalContent: pass isOpen and animation configuration
ModalContent->>ModalBox: pass animation state
ModalContent->>Backdrop: pass animation visibility
ModalBox-->>User: render modal transition
Backdrop-->>User: render backdrop transition
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/react-tokens/scripts/generateTokens.mjs (1)
36-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalization approach is correct but inconsistent with
build-single-packages.mjs.
formatFilePathToNameusesfilePath.replace(/\\/g, '/')whilebuild-single-packages.mjsusescomponent.replaceAll('\\', '/'). Both achieve the same result, but the inconsistency could confuse maintainers. Consider standardizing on one approach across both scripts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-tokens/scripts/generateTokens.mjs` around lines 36 - 43, Standardize path-separator normalization between formatFilePathToName and build-single-packages.mjs by using the same replacement approach in both scripts; update formatFilePathToName’s normalizedPath logic to match the established approach used by build-single-packages.mjs.packages/react-core/src/components/Modal/ModalContent.tsx (1)
123-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
focusTrapActiveto a singleconstexpression.The
let+ifreassignment can be expressed as a single declaration, which is more concise and avoids mutable state.♻️ Proposed refactor
- let focusTrapActive = !disableFocusTrap; - if (hasAnimations) { - focusTrapActive = !disableFocusTrap && isOpen; - } + const focusTrapActive = !disableFocusTrap && (!hasAnimations || isOpen);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Modal/ModalContent.tsx` around lines 123 - 126, Replace the mutable focusTrapActive initialization and conditional reassignment in the ModalContent component with a single const boolean expression that preserves the existing disableFocusTrap, hasAnimations, and isOpen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Modal/examples/ModalAnimated.tsx`:
- Around line 16-40: ModalAnimated’s Modal configuration is missing an onClose
handler, preventing Escape-key and header close-button dismissal. Add
onClose={handleModalToggle} to the Modal component in the ModalAnimated example
while preserving the existing footer button handlers.
In `@packages/react-core/src/components/Modal/examples/ModalAnimatedProvider.tsx`:
- Around line 25-53: Add onClose={handleModalToggle} to the Modal component in
the ModalAnimatedProvider example so the header close button and Escape key both
dismiss the modal consistently with the existing button handlers.
In `@packages/react-core/src/components/Modal/ModalContent.tsx`:
- Around line 77-84: Animated modal content remains mounted after closing
because ModalContent only skips rendering when animations are disabled. Add
close-animation completion handling around useHasAnimations and the isOpen state
so the component unmounts after the exit transition finishes, using a
transition-end callback or timeout fallback, while preserving immediate null
rendering for non-animated closes.
In
`@packages/react-styles/src/css/components/ModalAnimations/modal-animations.css`:
- Around line 14-22: Update the transition declaration in
.pf-v6-c-modal-animated-open to include opacity alongside transform and
visibility, preserving the existing timing and easing so opening the modal fades
it in as well as moving it.
---
Nitpick comments:
In `@packages/react-core/src/components/Modal/ModalContent.tsx`:
- Around line 123-126: Replace the mutable focusTrapActive initialization and
conditional reassignment in the ModalContent component with a single const
boolean expression that preserves the existing disableFocusTrap, hasAnimations,
and isOpen behavior.
In `@packages/react-tokens/scripts/generateTokens.mjs`:
- Around line 36-43: Standardize path-separator normalization between
formatFilePathToName and build-single-packages.mjs by using the same replacement
approach in both scripts; update formatFilePathToName’s normalizedPath logic to
match the established approach used by build-single-packages.mjs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6deb22-1566-4953-ad5c-1f8f2fa05074
📒 Files selected for processing (11)
packages/react-core/src/components/Backdrop/Backdrop.tsxpackages/react-core/src/components/Modal/Modal.tsxpackages/react-core/src/components/Modal/ModalBox.tsxpackages/react-core/src/components/Modal/ModalContent.tsxpackages/react-core/src/components/Modal/examples/Modal.mdpackages/react-core/src/components/Modal/examples/ModalAnimated.tsxpackages/react-core/src/components/Modal/examples/ModalAnimatedProvider.tsxpackages/react-styles/src/css/components/BackdropAnimations/backdrop-animations.csspackages/react-styles/src/css/components/ModalAnimations/modal-animations.csspackages/react-tokens/scripts/generateTokens.mjsscripts/build-single-packages.mjs
| hasAnimations: hasAnimationsProp, | ||
| ...props | ||
| }: ModalContentProps) => { | ||
| if (!isOpen) { | ||
| const hasAnimations = useHasAnimations(hasAnimationsProp); | ||
|
|
||
| if (!isOpen && !hasAnimations) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/react-core/src/components/Modal/*' | sed 's#^`#FILE` #' | sort
echo '--- ModalContent.tsx ---'
cat -n packages/react-core/src/components/Modal/ModalContent.tsx
echo '--- search for transition/end handlers and unmount flags ---'
rg -n "onTransitionEnd|onAnimationEnd|shouldRender|setShouldRender|hasAnimations|useHasAnimations|ModalContent" packages/react-core/src/components/Modal -SRepository: patternfly/patternfly-react
Length of output: 14629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Modal.tsx ---'
cat -n packages/react-core/src/components/Modal/Modal.tsx
echo '--- ModalBox.tsx ---'
cat -n packages/react-core/src/components/Modal/ModalBox.tsx
echo '--- ModalContent.test.tsx relevant assertions ---'
sed -n '1,220p' packages/react-core/src/components/Modal/__tests__/ModalContent.test.tsx | cat -n
echo '--- search docs/examples for closing animation / unmount ---'
rg -n "closing animation|remove the modal|hasAnimations|isOpen|unmount|transition end|animation end" packages/react-core/src/components/Modal -SRepository: patternfly/patternfly-react
Length of output: 22854
Animated modals still never unmount on close packages/react-core/src/components/Modal/ModalContent.tsx:82-84
With hasAnimations, ModalContent keeps rendering after isOpen becomes false, and there’s no transition-end or timeout path to remove it after the close animation. The modal stays mounted until the parent unmounts it, so the “remove the modal after the closing animation completes” behavior is still missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Modal/ModalContent.tsx` around lines 77 -
84, Animated modal content remains mounted after closing because ModalContent
only skips rendering when animations are disabled. Add close-animation
completion handling around useHasAnimations and the isOpen state so the
component unmounts after the exit transition finishes, using a transition-end
callback or timeout fallback, while preserving immediate null rendering for
non-animated closes.
There was a problem hiding this comment.
Deferring this to discuss with PF team during office-hours
| .pf-v6-c-modal-animated-open { | ||
| --pf-v6-c-modal-animated--Transition: | ||
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | ||
| visibility 0ms linear 0ms; | ||
| opacity: 1; | ||
| visibility: visible; | ||
| pointer-events: unset; | ||
| transform: translate3d(0, 0, 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Opening animation does not fade in — opacity missing from transition in .pf-v6-c-modal-animated-open.
The open class overrides --pf-v6-c-modal-animated--Transition with only transform and visibility, omitting opacity. When the browser applies the open class, it uses the new transition value for all property changes. Since opacity is not listed, it jumps instantly from 0 to 1 — no fade-in effect. The closing animation works correctly because the base class transition (which includes opacity) applies when the closed class is used. This asymmetry contradicts the PR objective: "Fade and shift the modal slightly downward when opening."
🐛 Proposed fix: add opacity to the open transition
.pf-v6-c-modal-animated-open {
--pf-v6-c-modal-animated--Transition:
+ opacity 240ms cubic-bezier(0, 0, 0.2, 1),
transform 240ms cubic-bezier(0, 0, 0.2, 1),
visibility 0ms linear 0ms;
opacity: 1;
visibility: visible;
pointer-events: unset;
transform: translate3d(0, 0, 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .pf-v6-c-modal-animated-open { | |
| --pf-v6-c-modal-animated--Transition: | |
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | |
| visibility 0ms linear 0ms; | |
| opacity: 1; | |
| visibility: visible; | |
| pointer-events: unset; | |
| transform: translate3d(0, 0, 0); | |
| } | |
| .pf-v6-c-modal-animated-open { | |
| --pf-v6-c-modal-animated--Transition: | |
| opacity 240ms cubic-bezier(0, 0, 0.2, 1), | |
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | |
| visibility 0ms linear 0ms; | |
| opacity: 1; | |
| visibility: visible; | |
| pointer-events: unset; | |
| transform: translate3d(0, 0, 0); | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 18-18: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/react-styles/src/css/components/ModalAnimations/modal-animations.css`
around lines 14 - 22, Update the transition declaration in
.pf-v6-c-modal-animated-open to include opacity alongside transform and
visibility, preserving the existing timing and easing so opening the modal fades
it in as well as moving it.
There was a problem hiding this comment.
Deferring this to discuss with PF team during office-hours
There was a problem hiding this comment.
@GAUNSD, understood — deferring this to the PF team office-hours discussion makes sense given it touches the broader animation design intent. Happy to revisit once you have their input; feel free to ping if the direction changes.
- Add missing onClose handler to ModalAnimated example - Add missing onClose handler to ModalAnimatedProvider example Generated-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
/deploy-preview |
|
Preview: https://pf-react-pr-12552.surge.sh A11y report: https://pf-react-pr-12552-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
In addition to the file comment below, can you move the styles that you added to the react-styles directory to the Modal > examples directory? There's an issue where those styles arent being applied, and due to the update to ModalContent's return null check, it's always rendering the Modal as visible and unable to close it.
What I did locally was create a Modal.css file in Modal > examples, copy+pasted the contents of backdrop-animations.css and modal-animations.css from react-styles into it, then add a import './Modal.css'; line in the Modal.md file. This should show the animations correctly for the preview deployment.
| import { Fragment, useRef, useState } from 'react'; | ||
| import RhUiWarningIcon from '@patternfly/react-icons/dist/esm/icons/rh-ui-warning-icon'; | ||
| import RhUiAttentionBellFillIcon from '@patternfly/react-icons/dist/esm/icons/rh-ui-attention-bell-fill-icon'; | ||
| import RhUiQuestionMarkCircleIcon from '@patternfly/react-icons/dist/esm/icons/rh-ui-question-mark-circle-icon'; |
There was a problem hiding this comment.
This line will need to be added back. The icon is being used in one of the Modal examples, and we need to include those same import lines in these MD files.
There was a problem hiding this comment.
My bad! Not sure why these imports got modified on my end - could've been a weird Windows build issue. Adding them back now!
|
Thanks @thatblindgeye! I've pushed up the changes requested. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Modal/examples/Modal.css`:
- Around line 24-32: Update the transition custom property in
`.pf-v6-c-modal-animated-open` and its production counterpart so it includes the
existing 240ms opacity transition along with transform and visibility,
preserving the modal’s fade-and-shift opening animation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 345da619-f03a-41f8-b69e-fca3beac3b45
📒 Files selected for processing (4)
packages/react-core/src/components/Modal/examples/Modal.csspackages/react-core/src/components/Modal/examples/Modal.mdpackages/react-core/src/components/Modal/examples/ModalAnimated.tsxpackages/react-core/src/components/Modal/examples/ModalAnimatedProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-core/src/components/Modal/examples/ModalAnimatedProvider.tsx
- packages/react-core/src/components/Modal/examples/ModalAnimated.tsx
- packages/react-core/src/components/Modal/examples/Modal.md
| .pf-v6-c-modal-animated-open { | ||
| --pf-v6-c-modal-animated--Transition: | ||
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | ||
| visibility 0ms linear 0ms; | ||
| opacity: 1; | ||
| visibility: visible; | ||
| pointer-events: unset; | ||
| transform: translate3d(0, 0, 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing opacity transition in the open state — modal won't fade in.
The .pf-v6-c-modal-animated-open class overrides --pf-v6-c-modal-animated--Transition with only transform and visibility, omitting opacity. Since the base class sets transition: var(--pf-v6-c-modal-animated--Transition), the resolved transition when opening excludes opacity, causing it to snap from 0→1 instantly instead of fading in. This directly contradicts the PR objective of "fade and shift the modal slightly downward when opening."
The closing path works correctly because .pf-v6-c-modal-animated-closed doesn't override the custom property, so the base transition (which includes opacity 240ms) applies.
The same fix should be applied to the production file packages/react-styles/src/css/components/ModalAnimations/modal-animations.css.
✨ Proposed fix: add opacity to the open-state transition
.pf-v6-c-modal-animated-open {
--pf-v6-c-modal-animated--Transition:
+ opacity 240ms cubic-bezier(0, 0, 0.2, 1),
transform 240ms cubic-bezier(0, 0, 0.2, 1),
visibility 0ms linear 0ms;
opacity: 1;
visibility: visible;
pointer-events: unset;
transform: translate3d(0, 0, 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .pf-v6-c-modal-animated-open { | |
| --pf-v6-c-modal-animated--Transition: | |
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | |
| visibility 0ms linear 0ms; | |
| opacity: 1; | |
| visibility: visible; | |
| pointer-events: unset; | |
| transform: translate3d(0, 0, 0); | |
| } | |
| .pf-v6-c-modal-animated-open { | |
| --pf-v6-c-modal-animated--Transition: | |
| opacity 240ms cubic-bezier(0, 0, 0.2, 1), | |
| transform 240ms cubic-bezier(0, 0, 0.2, 1), | |
| visibility 0ms linear 0ms; | |
| opacity: 1; | |
| visibility: visible; | |
| pointer-events: unset; | |
| transform: translate3d(0, 0, 0); | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 28-28: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Modal/examples/Modal.css` around lines 24
- 32, Update the transition custom property in `.pf-v6-c-modal-animated-open`
and its production counterpart so it includes the existing 240ms opacity
transition along with transform and visibility, preserving the modal’s
fade-and-shift opening animation.
There was a problem hiding this comment.
Deferring this to discuss with PF team during office-hours
There was a problem hiding this comment.
@GAUNSD, understood — thanks for flagging that this will be discussed with the PF team during office-hours. I'll leave this open for now and won't follow up further until there's a decision from that discussion.
|
/deploy-preview |
4 participants
What:
Additional issues:
Summary
This PR adds opt-in animations to the
<Modal>, and<Backdrop>components.Evidence
Video: Modal animations in doc page as new example
modal-animations-standard.mp4
Video: Slowed down animations to 25% speed
modal-animations-slow.mp4
Detail:
ModalBox'sreturn nullNotably; the ModalBox was previously returning null which made the animation less desirable since the entire modal content was not rendered.
Now the ModalBox will skip this and only
return nullwhen animations are not set.Detail:
modal-animations.css&backdrop-animations.cssJust to get this change easily-tested and off the ground I've added the styles directly into the react-styles package.
Ideally these styles would live upstream in patternfly/modal-box.scss and other relevant areas.
Detail: Build tweaks to
generateTokens.mjs&build-single-packages.mjsI'm running locally on Windows where path separators are
\instead of the/.While running locally, some build scripts fail because of this and tweaks were made in order to get a successful build.
Summary by CodeRabbit
prefers-reduced-motionhandling to disable animations for reduced-motion users.