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

Pull request #103

Merged
merged 10 commits into from
May 23, 2018
Merged

Pull request #103

merged 10 commits into from
May 23, 2018

Conversation

LarsKumbier
Copy link
Contributor

Adds the ability to change the swipe rotation for rotated components

In one of our projects, we used the react-image-gallery, but needed to rotate it for different views. Since the swipe component react-swipeable was unable to rotate the swipes accordingly, we added the appropriate parts in this PR. Changes are non-breaking.

Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! What an interesting requirement.

Just a few thoughts.

src/Swipeable.js Outdated
@@ -25,8 +25,15 @@ function getPosition(e) {
: { x: e.clientX, y: e.clientY };
}

function rotateByAngle({ x, y }, angle) {
const angleInRadians = (Math.PI / 180) * angle;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on quick out?

function rotateByAngle(positions, angle) {
  if (angle === 0) return positions;

and don't destructure, since that will be the majority, currently, of use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add that

@@ -89,6 +89,8 @@ as well as the x distance, + or -, from where the swipe started to where it ende

**`innerRef`** will allow access to the Swipeable's inner dom node element react ref. See [#81](https://github.com/dogfessional/react-swipeable/issues/81) for more details. Example usage `<Swipeable innerRef={(el) => this.swipeableEl = el} >`. Then you'll have access to the dom element that Swipeable uses internally.

**`rotationAngle`** will allow to set a rotation angle, e.g. for a four-player game on a tablet, where each player has a 90° turned view. The default value is `0`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we "lock" this to a range of 0 - 360? via a new proptype?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We thought about the same, but in the end it does not matter for the calculation - so why lock the range.

@@ -379,4 +380,125 @@ describe('Swipeable', () => {
expect(swipeableDiv.prop('onTouchMove')).toBe(instance.eventMove);
});
});

describe('Handle Rotation by 90 degree: ', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding tests!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'course - won't work without. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, a question: the tests are mainly data-driven. Is it okay to refactor them into data-driven tests with less duplication?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for cleaning up the tests and if you'd like to do that, could it be another PR?

Copy link
Contributor Author

@LarsKumbier LarsKumbier May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, external PR would be best

@hartzis hartzis self-assigned this May 14, 2018
@LarsKumbier
Copy link
Contributor Author

@hartzis Could you re-review? Added more tests for negative degrees and over-360-degrees for documentation and the early-return as requested.

Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! i hope to get this merged and versioned within the week!

@hartzis hartzis merged commit 386417f into FormidableLabs:master May 23, 2018
@LarsKumbier LarsKumbier deleted the pull-request branch May 23, 2018 14:00
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants