feat(AnimationsProvider): Add Support for AnimationsProvider #11990
feat(AnimationsProvider): Add Support for AnimationsProvider #11990wise-king-sullyman merged 5 commits intopatternfly:mainfrom
Conversation
|
Preview: https://pf-react-pr-11990.surge.sh A11y report: https://pf-react-pr-11990-a11y.surge.sh |
…ons, add AnimationsProvider to helpers, add md files for documentation fix(components): update hasAnimations to fall back to context fix(md): remove cssPrefix fix(alertgroup): fix tests
855d346 to
49bcb04
Compare
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Great work on this! I have a handful of comments/change requests but they're mostly fairly small things.
This review didn't include looking at changing AlertGroup and DualListSelector from class based to function components that was needed as unfortunately I ran out of time. I wouldn't say to redo anything, but just for the sake of the future if a similar situation arises I would advocate for splitting that work into a distinct PR just to make review/testing easier, but that's just my $0.02.
packages/react-core/src/components/Alert/__tests__/AlertGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Alert/__tests__/AlertGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/DualListSelector/DualListSelector.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/AnimationsProvider.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/AnimationsProvider.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/__tests__/AnimationsProvider.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/__tests__/AnimationsProvider.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/examples/AnimationsProvider.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/examples/AnimationsProviderBasic.tsx
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| // Animation context tests | ||
| test('respects AnimationsProvider context when no local hasAnimations prop', () => { |
There was a problem hiding this comment.
Kind of the opposite of Austin's comment about regarding a describe block, I personally might wrap these tests in a describe block specifically for the context tests. Not a blocker and depending how quickly we want to get this merged in, would like to hear @wise-king-sullyman opinion here.
There was a problem hiding this comment.
I think so too @thatblindgeye - based on the RTL guide this seems like a good use case for a grouped describe() block, I've updated the tests to include it, but will also defer to @wise-king-sullyman here for the final word on whether or not we'd like to keep it.
There was a problem hiding this comment.
I'm not against grouping associated things like the animation tests together in a describe block in an otherwise broader test suite, just wrapping full test suites in a describe block 🙂
| target.removeChild(this.state.container); | ||
| } | ||
| } | ||
| appendTo, // do not pass down to ul |
There was a problem hiding this comment.
Nit: it's pre-existing but I think we could remove this comment now that it's a functional component.
There was a problem hiding this comment.
Nice catch, removed
packages/react-core/src/components/DualListSelector/DualListSelector.tsx
Outdated
Show resolved
Hide resolved
| const [focusAfterExpandChange, setFocusAfterExpandChange] = useState(false); | ||
|
|
||
| const { isExpanded, onToggleExpand, toggleAriaLabel, hasAnimations } = expandableInput || {}; | ||
| const { isExpanded, onToggleExpand, toggleAriaLabel, hasAnimations: localHasAnimations } = expandableInput || {}; |
There was a problem hiding this comment.
Just as a followup to my above reply to Austin about the prop name, I think this makes sense to call it localHasAnimations.
There was a problem hiding this comment.
Just following up here to confirm based on the discussion in the thread here, I renamed it to hasAnimations for both the destructed and internal variable for consistency with our existing conventions for destructured props, for example JumpLinks uses activeIndex: activeIndexProp and isExpanded: isExpandedProp. What are your thoughts on using hasAnimationsProp here?
…e prop to hasAnimationsProp to match existing conventions
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #11962
This PR enables support for
AnimationsProvider.PatternFly consumers will now be able to set animations for opt-in components once at the app level (instead of on every component via
hasAnimations) while maintaining full backward compatibility.Testing Instructions
To test the new context provider functionality:
Run
yarn startand navigate to http://localhost:8002/components/alert#toast-alert-groupCopy and paste the code below into the demo TSX code:
Click to expand
hasAnimationsprop.Additional Changes & Fixes
Converted class components to functional components to enable React Hooks usage
useHasAnimationshook integration:AlertGroup.tsx- Required foruseHasAnimationshook integrationDualListSelector.tsx- Required foruseHasAnimationshook integrationQuick Verification
hasAnimationsprop.<AnimationsProvider config={{ hasAnimations: true }}>hasAnimationsprop from the child component.Testing Note: Since PatternFly example files don't typically import from
/helpers, the ESLint configs require aReactimport with the// eslint-disable-next-line no-restricted-importscomment at the top of the file for local testing. Real consumers won't have this issue.Success Criteria
hasAnimationsprop overrides provider when set