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

fix(Carousel): fixes #4136 - reverse carousel slide navigation direction for touch #4137

Merged

Conversation

@RageJS
Copy link
Contributor

RageJS commented Jul 28, 2019

See issue 4136

@taion
taion approved these changes Jul 28, 2019
src/Carousel.js Outdated Show resolved Hide resolved
src/Carousel.js Outdated Show resolved Hide resolved
@taion

This comment has been minimized.

Copy link
Member

taion commented Jul 28, 2019

The changes are fine, but please accept the updates to the comments, and fix the code formatting so CI passes.

@RageJS

This comment has been minimized.

Copy link
Contributor Author

RageJS commented Jul 28, 2019

@taion Made the changes as per PR suggestions.

@bpas247

This comment was marked as outdated.

Copy link
Member

bpas247 commented Jul 29, 2019

it('should swipe left', () => {
try {
wrapper.simulate('touchStart', { changedTouches: [{ screenX: 50 }] });
wrapper.simulate('touchEnd', { changedTouches: [{ screenX: 0 }] });
clock.tick(50);
expect(onSelectSpy).to.have.been.calledOnce;
expect(onSelectSpy.getCall(0).args[0]).to.equal(1);
} finally {
clock.restore();
}
});
it('should swipe right', () => {
try {
wrapper.simulate('touchStart', { changedTouches: [{ screenX: 0 }] });
wrapper.simulate('touchEnd', { changedTouches: [{ screenX: 50 }] });
clock.tick(50);
expect(onSelectSpy).to.have.been.calledOnce;
expect(onSelectSpy.getCall(0).args[0]).to.equal(1);
} finally {
clock.restore();
}
});

I'm thinking we'll need to fix these tests so that they're actually testing if it's swiping left or right. I think right now it's only really testing if it changes to the other Carousel.Item component or not when it's swiped (since there are only two items in the Carousel for those tests).

@RageJS

This comment has been minimized.

Copy link
Contributor Author

RageJS commented Jul 29, 2019

@bpas247 I think the code you're checking, that's from current master branch. In my PR, I had changed that file to have three carousel items, and checked swipe direction accordingly. Can be checked here, https://github.com/react-bootstrap/react-bootstrap/pull/4137/files

@bpas247 bpas247 merged commit 5be68b7 into react-bootstrap:master Jul 29, 2019
5 checks passed
5 checks passed
codecov/patch 100% of diff hit (target 94.24%)
Details
codecov/project Absolute coverage decreased by -0.15% but relative coverage increased by +5.75% compared to fce0e8b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@RageJS RageJS deleted the RageJS:fix/carousel-touch-slide-direction branch Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.