Skip to content

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