-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow List.Accordion to behave as a controlled component #638
Conversation
The PR was causing an additional problem with the lint not passing, the linebreak has been removed causing it
Hey @vieiralucas, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API. |
I tried my best to improve the docs so the user understands that he needs to pass Thanks |
this.setState(state => ({ | ||
expanded: !state.expanded, | ||
})); | ||
} |
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.
Can I propose a little refactor for this method?
const onPressIsFunction = typeof this.props.onPress === 'function';
const expandedPropIsDefined = this.props.expanded !== undefined;
if (expandedPropIsDefined) {
if (!onPressIsFunction) {
throw new Error(
'The `onPress` prop must be passed when the `expanded` prop is passed to List.Accordion.'
);
}
this.props.onPress();
return;
}
if (onPressIsFunction) {
this.props.onPress();
}
this.setState(state => ({
expanded: !state.expanded,
}));
Motivation
This PR aims to FIX #616 and FIX #635 and CLOSE #618
It allows the List.Accordion to behave as a controlled component by passing a
expanded
andonPress
prop.This also enables users to start the accordion as expanded.
Test plan
I added a controlled List.Accordion to the example app and also added a snapshot test.
Thank you @jimmiehansson for starting this out in #618, I left your commits on my branch so you can take your deserved credits ❤️