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

Make mobile gestures more user friendly #399

Merged
merged 4 commits into from
Mar 21, 2017

Conversation

mykola-mokhnach
Copy link
Contributor

Trying to decrease the count of GitHub issues about iOS gestures and make the stuff more user-frindly in general.

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

LOVE this reorganization and the clarity it should provide to users.

if (gestures.length !== candidates.length) {
return false;
}
for (let i in gestures) {
Copy link
Member

Choose a reason for hiding this comment

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

we don't use for ... in, we use for ... of; or in the case of wanting the index, just a regular for loop:

for (let i = 0; i < gestures.length; i++) {

Copy link
Member

Choose a reason for hiding this comment

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

i also wonder if you could wrap this whole for block in a try/catch; if you get an exception anywhere in here it must mean they are different as well, right?

}

function gesturesChainToString (gestures) {
return _.map(gestures, (item) => {
Copy link
Member

Choose a reason for hiding this comment

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

no need to use _.map, can just do:

return gestures.map((item) => {

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

good to go now. of course a few unit tests of the isSameGestures and gesturesChainToString methods might be nice.

@mykola-mokhnach
Copy link
Contributor Author

@jlipps how it would be better to export these helper functions, so they are available for unit testing?

@@ -24,21 +24,102 @@ commands.click = async function (el) {
}
};

function isSameGestures (gestures, candidates) {
Copy link
Member

Choose a reason for hiding this comment

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

yeah, you can just make this helpers.isSameGestures = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jlipps
Copy link
Member

jlipps commented Mar 16, 2017

I think @imurchie should have the chance to weigh in, otherwise it looks ready to merge

@@ -24,21 +24,104 @@ commands.click = async function (el) {
}
};

helpers.isSameGestures = function (gestures, candidates) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function does not use anything on the driver object, I would rather see it (and the gesturesChainToString function) as a standalone function, which is then exported from this file for testing purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you please suggest on how to properly import these functions from unit tests in such case?

Copy link
Contributor

Choose a reason for hiding this comment

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

// lib/commands/gesture.js
// ...
function isSameGestures (gestures, candidates) {
  // ...
}

// ...

Object.assign(extensions, helpers, commands);
export { extensions, helpers, commands, isSameGestures };
export default extensions;
// test/unit/commands/gesture-specs.js
// ...
import { isSameGestures } from '../../../lib/commands/gesture';

// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. That's exactly what I needed

@imurchie imurchie merged commit 93544dd into appium:master Mar 21, 2017
@imurchie
Copy link
Contributor

Published in appium-xcuitest-driver@2.23.1.

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