Skip to content

Conversation

beckerei
Copy link

@beckerei beckerei commented Jul 8, 2019

Prior to version 5 all additional props besides the ones used by react-swipeable were passed down. It's not listed as breaking change, so I'm not sure if was removed on purpose.
I assume it was not, and since Swipeable will still render an actual div it makes sense to pass all props down to it.

PS: Breaking the max build size wit it 🎉 :)

@hartzis
Copy link
Collaborator

hartzis commented Jul 9, 2019

Good catch about the removal of spreading the props. You're absolutely right that should have been documented. And I'll try to get that updated asap, issue to track #154

@beckerei If you could add some more info to the issue I'd be grateful.

Initially, I'm leaning against adding this back in due to how babel compiles it which increases the bundle size. Though I do need to add the breaking change to the changelog, thank you again for noticing that!

There are also "simple" workarounds by just adding another div with your required attributes?

Maybe there is a "better" way that doesn't add so much to the size? the delete way that v4 used?

https://github.com/dogfessional/react-swipeable/blob/637ef023fa8115ba1687567d210c9b8d48c33224/src/Swipeable.js#L292-L293

Please let me know your thoughts. I genuinely appreciate your PR and the time you've committed already.

@hartzis
Copy link
Collaborator

hartzis commented Jul 9, 2019

I'm also don't know exactly if this would constitute another major as it technically could break stuff and could have unintended consequences. Thoughts?

@beckerei
Copy link
Author

If you are concerned about bundle size I think it's okay to not add this functionality back. Adding this as breaking in the changelog is sufficient.

There are also "simple" workarounds by just adding another div with your required attributes?

That's how we handle it after we noticed, so no big deal.

I'm also don't know exactly if this would constitute another major as it technically could break stuff and could have unintended consequences. Thoughts?

Yes, technically it is a major/breaking change. Semver is only usefully when a potentially breaking change results in a major version.
Not adding the functionality back however would require no change at all :)

@hartzis
Copy link
Collaborator

hartzis commented Nov 29, 2019

Closing for now. As wrapping div can handle most cases.

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.

2 participants