fix(ExpandableSection): made animation opt-in for detached variant#11851
fix(ExpandableSection): made animation opt-in for detached variant#11851nicolethoen merged 6 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-11851.surge.sh A11y report: https://patternfly-react-pr-11851-a11y.surge.sh |
|
First commit shows the original logic where a new Second commit is more true opt-in where isDetached doesn't do anything new unless hasDetachedAnimations is also passed in. Note that I didn't update the direction prop and also didnt remove the pf-m-detached class from the Toggle component file; both of those I can update if this is the route we wanna go with for now |
| className={css( | ||
| styles.expandableSectionToggleIcon, | ||
| isExpanded && direction === 'up' && styles.modifiers.expandTop | ||
| isExpanded && direction === 'up' && styles.modifiers.expandTop // TODO: next breaking change move this class to the outter styles.expandableSection wrapper |
There was a problem hiding this comment.
Outer misspelled, and is there an issue open for this?
|
|
||
| When passing the `isDetached` property into `<ExpandableSection>`, you must also manually pass in the same `toggleId` and `contentId` properties to both `<ExpandableSection>` and `<ExpandableSectionToggle>`. This will link the content to the toggle via ARIA attributes. | ||
|
|
||
| By default animations will not be enabled for a detached `<ExpandableSection>`. You must manually pass the `direction` property with an appropriate value based on where the expandable content is rendered. If the expandable content is above the expandable toggle, `direction="up"` must be passed like in this example. |
There was a problem hiding this comment.
I don't mind the hasDetachedAnimations approach if we want to be more true to opt-in, but my main concern is that with how core is currently set up animations will be on by default and be potentially the incorrect direction for detached sections until updated (because pf-m-detached is what is disabling animations from detached and that won't be present until the flag is enabled).
I wonder if it would be better to make animations as a whole opt-in for expandable section with a new modifier instead, and as part of that also require the direction to be passed in for detached?
thatblindgeye
force-pushed
the
expSecDetachedAnims
branch
from
7317001 to
c458f96
Compare
|
Latest commit reflects convo during retroplanning yesterday where we decided to try updating the Core PR to target the |
thatblindgeye
force-pushed
the
expSecDetachedAnims
branch
from
4af8ab7 to
300bfd7
Compare
thatblindgeye
requested review from
a team,
rebeccaalpert and
wise-king-sullyman
and removed request for
a team
| /** Callback function to toggle the expandable content. */ | ||
| onToggle?: (isExpanded: boolean) => void; | ||
| /** Flag indicating that the expandable section and expandable toggle are detached from one another. */ | ||
| isDetached?: boolean; |
There was a problem hiding this comment.
Per convo with Coker, adding the pf-m-detached classes in both ExpandableSection components but making them opt-in for now. Inn a breaking change we'd want to apply the class by default for the toggle here and apply it only when isDetached is applied to ExpandableSection (will open a followup for this)
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
I'm reaching for straws here since this looks good to me, but just one nit you're welcome to ignore.
| styles.expandableSection, | ||
| isExpanded && styles.modifiers.expanded, | ||
| hasTruncatedContent && styles.modifiers.truncate, | ||
| isDetached && 'pf-m-detached', |
There was a problem hiding this comment.
Do we want styles.modifiers.detached instead? Just a nit though.
There was a problem hiding this comment.
Ah so right now that token doesn't exist yet since the class isn't being used in the CSS. If we update the CSS in the next breaking change to target this class specifically (rather than right now we're targeting based on the content being the only child), we should be able to use the styles.modifier.detached
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #11850
Additional issues: