- 
                Notifications
    You must be signed in to change notification settings 
- Fork 373
feat(ExpandableSection): functional toggleContent #12063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(ExpandableSection): functional toggleContent #12063
Conversation
| Preview: https://pf-react-pr-12063.surge.sh A11y report: https://pf-react-pr-12063-a11y.surge.sh | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, especially as a means to set the toggle content for controlled or uncontrolled implementations.
I think ultimately maybe it depends what we want the API to look like in the future:
- Would we want to have the API set in such a wway that a consumer only needs to pass toggleContent, and we remove toggleText, toggleTextExpanded, and toggleTextCollapsed? One prop to rule them all etc
- Or would we want a prop intended for a controlled implementation (toggleContent), and have 2 props for uncontrolled implementations (replacing toggleTextExpanded and toggleTextCollapsed with new props, toggleContentExpanded and toggleContentCollapsed) that accept ReactNodes as types)? In addition to removing toggleText as a prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm.
As far as future API, I'd be in favor of slimming down the props in the next breaking release - we probably could do away with toggleText in favor of toggleContent and deprecate the expanded/collapsed text props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a breaking change followup for the API: #12104
| @gitdallas just a merge conflict that needs resolving | 
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
f7a25e9    to
    b56de1b      
    Compare
  
    
What: Closes #12030