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

Move iOS Device rotation to generated code #727

Merged
merged 3 commits into from
May 26, 2018

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented May 12, 2018

No description provided.

@DanielMSchmidt DanielMSchmidt requested a review from rotemmiz as a code owner May 12, 2018 15:33
@rotemmiz rotemmiz changed the title Move device rotation to generated code [WIP] Move device rotation to generated code May 13, 2018
@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/ios-driver-to-generated-code branch from dd0fdb4 to 7210b3f Compare May 16, 2018 17:40
@DanielMSchmidt DanielMSchmidt requested a review from LeoNatan as a code owner May 16, 2018 17:40

function sanitize_uiDeviceOrientation(value) {
const orientationMapping = {
landscape: 3, // top at left side landscape
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why not expose all options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a refactoring, I would rather improve the situation a different PR :)

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 could create an issue, it's probably a very good first issue 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t understand. We have no usage of Earl Grey rotation so far. If this PR introduces it, why do we need additional issues or PRs?

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 have and this is the actual mapping we are currently using: https://github.com/wix/detox/blob/master/detox/src/devices/IosDriver.js#L59-L62

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was my first bigger PR in detox 😻

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh I confused rotate with shake.
In either case, what blocks the full enum being exposed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not much, I just would like to first transfer it and afterwards add the new cases. That way I surely don't touch tests and refactorings in one PR

@DanielMSchmidt DanielMSchmidt changed the title [WIP] Move device rotation to generated code Move device rotation to generated code May 16, 2018
@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/ios-driver-to-generated-code branch from 7210b3f to f030712 Compare May 21, 2018 06:35
@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/ios-driver-to-generated-code branch from f030712 to d52fd53 Compare May 26, 2018 08:16
@DanielMSchmidt DanielMSchmidt changed the title Move device rotation to generated code Move iOS Device rotation to generated code May 26, 2018
@rotemmiz rotemmiz merged commit 5025c6e into master May 26, 2018
@wix wix locked and limited conversation to collaborators Jul 23, 2018
@LeoNatan LeoNatan deleted the danielmschmidt/ios-driver-to-generated-code branch July 22, 2019 02:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants