Skip to content
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

Not using React state for swipe state #58

Merged
merged 2 commits into from
Jan 8, 2017

Conversation

grantila
Copy link
Contributor

The state used for x, y, swiping and start aren't used in the render methods (nothing is supposed to happen really, as it's up to the user of react-swipeable to perform any x/y-translations), and using setState() will trigger React logic to perform things which may cause stuttering and slow down transitions, etc.

This PR will save the state in a local variable this.swipable instead of the React state, to ensure we bypass all React state logic, since it's not needed.

@hartzis
Copy link
Collaborator

hartzis commented Nov 27, 2016

mind blown... how have we not noticed this before. you're absolutely correct. Everything wrapped by <Swipeable> is possibly being unnecessarily re-rendered a lot.

I have slight reservations as a recent issue made me realize people are probably reaching into this component and using the state, which this PR would 'break'.

@goatslacker if you have a second. Any thoughts on this?

@goatslacker
Copy link
Contributor

we can add a shouldComponentUpdate: () => false to prevent re-renders

@hartzis
Copy link
Collaborator

hartzis commented Dec 3, 2016

@goatslacker I liked your idea, but then dug around a bit and it seems shouldComponentUpdate returning false would stop children from re-rendering too.

Example, in react-image-gallery, {slides} here would not re-render with shouldComponentUpdate in <Swipeable> returning false.

Discussion - facebook/react#6515

After reading that it seems this PR is a simple approach and could possibly account for some performance gains.

@hartzis hartzis merged commit 7f07abc into FormidableLabs:master Jan 8, 2017
@github-actions github-actions bot mentioned this pull request May 18, 2023
@paulmarsicloud paulmarsicloud mentioned this pull request Jun 1, 2023
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