-
Notifications
You must be signed in to change notification settings - Fork 20
allow passing props to SwipeableList div #145
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
Conversation
}) => ( | ||
<div className={styles.swipeableList} data-testid="list-wrapper"> | ||
<div | ||
className={`${styles.swipeableList} ${className}`} |
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.
For missing className
prop (allowed by prop types) this would evaluate to className="someGeneratedClassName undefined"
. To avoid this kind of issue, a class names concatenation helper is needed - for example, https://sandstreamdev.github.io/std/web/classNames/ or https://www.npmjs.com/package/classnames.
In my opinion, in the future, more feature requests would require rendering to be customized, so I propose the component to allow headless rendering and just use user-provided render functions. This way the caller is responsible for the final rendering and the SwipeableList just provides its props there.
Consider API like this:
<SwipeableList>
{({ className, ...rest }) => (
<div className={classNames(className, 'scrollable')} {...rest}>
<SwipeableListItem>...<SwipeableListItem />
</div>
)}
</SwipeableList>
By default, it can provide the default renderer as it is, to keep compatibility. This is a very similar API to recently proposed one with #144 .
With the headless rendering, a hook based solution is also viable:
const { className, ...rest } = useSwipeableList();
return <div className={classNames(className, 'scrollable')} {...rest}>
<SwipeableListItem>...<SwipeableListItem />
</div>
I thought of using |
@marekrozmus what do you think about this? Either:
|
I think we could improve the #144 solution to add some additional stuff. I'll take care of it as soon as I finish those animations issues. Thanks a lot for improvements proposals 🙂 |
@all-contributors please add @MohammedFaragallah for ideas |
I've put up a pull request to add @MohammedFaragallah! 🎉 |
@MohammedFaragallah release with fixes is ready - enjoy 🎉 |
@marekrozmus thank you for your hard work and amazing project. I still can't pass additional props to SwipeableList, are you planning to add this feature? |
@MohammedFaragallah could you be more specific what kind of props? The requested styling container is now possible - check the "Custom container" example |
@marekrozmus sorry I didn't notice the custom container style I was talking about passing props to SwipeableList div but this also works and allows more flexibility thank you. |
I needed to make
overflow-y
onSwipeableList
hidden.