Skip to content

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

Closed
wants to merge 1 commit into from
Closed

allow passing props to SwipeableList div #145

wants to merge 1 commit into from

Conversation

MohammedFaragallah
Copy link

I needed to make overflow-y on SwipeableList hidden.

}) => (
<div className={styles.swipeableList} data-testid="list-wrapper">
<div
className={`${styles.swipeableList} ${className}`}
Copy link
Member

@przemyslawzalewski przemyslawzalewski Apr 8, 2020

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>

@MohammedFaragallah
Copy link
Author

I thought of using clsx but I didn't want to introduce a new dependency if it's ok I can make that change.

@przemyslawzalewski
Copy link
Member

@marekrozmus what do you think about this?

Either:

  • we include class names helper dependency (incurs a runtime & bundle penalty for all users, even if they do not override provided class names),
  • do the concatenation on our own (a simple [styles.swipeableList, className].filter(Boolean).join(' ') would be enough assuming className prop being just a string),
  • or do the headless rendering (the most generic way).

@marekrozmus
Copy link

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 🙂

@marekrozmus
Copy link

@all-contributors please add @MohammedFaragallah for ideas

@allcontributors
Copy link
Contributor

@marekrozmus

I've put up a pull request to add @MohammedFaragallah! 🎉

@marekrozmus
Copy link

@MohammedFaragallah release with fixes is ready - enjoy 🎉

@MohammedFaragallah
Copy link
Author

@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?

@marekrozmus
Copy link

@MohammedFaragallah could you be more specific what kind of props? The requested styling container is now possible - check the "Custom container" example

@MohammedFaragallah
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants