-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
* Track the x/y position of user mousing over screenshot * When in tap mode, and mousedown is applied, call the 'tap' event * Refactor main appium thread to handle TouchEvent case
* Added swiping interactions (set swipe start, set swipe end, set swipe duration and show/hide swipe duration prompt) * Clicking on the screenshot sets swipe start and swipe end * Added callbabck that applies the swipe method (this will need to be refactored on the main thread) * Uses SVG to show swipe start as a circle and swipe start + swipe end as a rounded line
* Added it to all the clients * Also, adjusted mousemove events to round x/y coordinates to 2 decimals
* Not using duration anymore, just doing a simple drag event
* Fix swipeSVG * In shared.js, fix promise not being rejected with Error * In Appium.js send a string back instead of an error object (this fixes the problem of undescriptive error messages * Fix conditional problem in appium-method-handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks really great! made some comments. in addition i'll want to play around with the css but we can probably merge this and do that later.
app/main/appium-method-handler.js
Outdated
@@ -80,7 +81,14 @@ export default class AppiumMethodHandler { | |||
|
|||
async executeMethod (methodName, args = []) { | |||
let res = {}; | |||
if (methodName !== 'source' && methodName !== 'screenshot') { | |||
|
|||
// Specially handle the tap method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't we specially handling the tap and swipe methods here?
app/main/appium-method-handler.js
Outdated
res = await (new wd.TouchAction(this.driver)).tap({x: args[0], y: args[1]}).perform(); | ||
} else if (methodName === 'swipe') { | ||
const [startX, startY, endX, endY] = args; | ||
res = await (new wd.TouchAction(this.driver)).press({x: startX, y: startY}).moveTo({x: endX, y: endY}).release().perform(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this would look cleaner separating out the individual commands on separate lines:
res = await (new wd.TouchAction(this.driver))
.press({x: startX, y: starty})
.moveTo({x: endX, y: endY})
.release()
.perform();
handleMouseOut () { | ||
this.setState({ | ||
...this.state, | ||
x: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like setting things to undefined, can we do null
instead?
@@ -89,20 +137,48 @@ export default class Screenshot extends Component { | |||
// If we're tapping or swiping, show the 'touch' cursor style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment here doesn't seem to match what we're doing
onMouseMove={this.handleMouseMove.bind(this)} | ||
onMouseOut={this.handleMouseOut.bind(this)} | ||
className={styles.screenshotBox}> | ||
{x !== undefined ? <div className={styles.coordinatesContainer}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check could then be a more natural null check
@@ -89,20 +137,48 @@ export default class Screenshot extends Component { | |||
// If we're tapping or swiping, show the 'touch' cursor style | |||
const screenshotStyle = {}; | |||
if (screenshotInteractionMode === 'tap' || screenshotInteractionMode === 'swipe') { | |||
screenshotStyle.cursor = 'crosshair'; // TODO: Change this to touch | |||
screenshotStyle.cursor = 'crosshair'; | |||
} | |||
|
|||
recursive(source); | |||
|
|||
// Show the screenshot and highlighter rects. Show loading indicator if a method call is in progress. | |||
return <Spin size='large' spinning={!!methodCallInProgress}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is now enough HTML code here I wonder if we should factor out some smaller components. up to you.
} | ||
|
||
codeFor_swipe (varNameIgnore, varIndexIgnore, x1, y1, x2, y2) { | ||
return `(new TouchAction(driver)).press({x: ${x1}, y: ${y1}}).moveTo({x: ${x2}: y: ${y2}}).release().perform()`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should also split this onto multiple lines and indent for the generated code as well.
67f8603
to
0e9ad30
Compare
Fixes made |
(https://drive.google.com/file/d/0ByQg0qBtDmn1N0hzUmZ3RXlYUXM/view?usp=sharing)