-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat(components/molecule/collapsible): Custom toggle button and butto… #2466
base: master
Are you sure you want to change the base?
Conversation
|
icon, | ||
showText, | ||
hideText, | ||
toggleButton: ToggleButton, |
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.
we prefer element over elementType
/** | ||
* Custom toggle button | ||
*/ | ||
toggleButton: PropTypes.node, |
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.
you are using it as a elementType, not a node
<button | ||
type="button" | ||
className={BUTTON_CLASS} | ||
onClick={toggleCollapse} | ||
> | ||
<span className={BUTTON_CONTENT_CLASS} tabIndex="-1"> | ||
{collapsed ? showText : hideText} | ||
<span className={iconClassName}>{icon}</span> | ||
</span> | ||
</button> |
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 part whould be componentized on a different file. Call it CollapseToogleButton
.
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.
lets schedule a pair programming session.
ea080b0
to
720268a
Compare
|
…n alignment
Category/Component
🔍 Show
TASK:
Types of changes
Description, Motivation and Context
We add two new features for this component:
This solves some UX requests made
Screenshots - Animations