Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Highlight elements that user searched for #344

Merged
merged 6 commits into from
Nov 21, 2017

Conversation

dpgraham
Copy link
Contributor

When the user opens the 'Search for Element' modal, and searches for elements, when they select one, highlight that element on the screenshots panel.

* Can select an element that was searched for and then sets the bounds in Redux
* Can also choose not to retrieve screenshot/source when calling a method
* HighlighterRects then passes the location and size of the element that was selected in 'Search for Element' modal
* Don't show other highlighters when Search Modal is open
* When an element is selected, clear the currently selected element before fetching the bounds for the new element
@@ -70,7 +70,10 @@ export default class AppiumMethodHandler {
// Give the source/screenshot time to change before taking the screenshot
await Bluebird.delay(500);

let sourceAndScreenshot = await this._getSourceAndScreenshot();
let sourceAndScreenshot;
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Nov 15, 2017

Choose a reason for hiding this comment

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

I assume an empty array should be assigned to this variable by default since we apply ellipsis operator to it later

Copy link
Contributor Author

@dpgraham dpgraham Nov 16, 2017

Choose a reason for hiding this comment

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

When you pass undefined or null into an object via ellipsis (spread) operator, it has no effect.

@@ -101,7 +104,11 @@ export default class AppiumMethodHandler {
// Give the source/screenshot time to change before taking the screenshot
await Bluebird.delay(500);

let sourceAndScreenshot = await this._getSourceAndScreenshot();
let sourceAndScreenshot;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid code duplication 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.

Yeah I'll make it so there's one main method.

@@ -301,6 +301,7 @@ function connectClientMethodListener () {
fetchArray = false, // Optional. Are we fetching an array of elements or just one?
elementId, // Optional. Element being operated on
args = [], // Optional. Arguments passed to method
skipScreenshot = false, // Optional. Do we want the updated source and screenshot?
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps, we can select better name for this variable if it also affects source update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call

@@ -377,11 +382,19 @@ export function findAndAssign (strategy, selector, variableName, isArray) {
};
}


// TODO: Is this obsolete? I don't think we use this.
export function setLocatorTestElement (elementId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this function be also defined as async?

Copy link
Contributor Author

@dpgraham dpgraham Nov 16, 2017

Choose a reason for hiding this comment

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

The function that is returned within it is async.

@dpgraham
Copy link
Contributor Author

Changes pushed

if (elementId) {
// Give the cached element a variable name (el1, el2, el3,...) the first time it's used
cachedEl = this.elementCache[elementId];
if (!cachedEl.variableName && cachedEl.variableType === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use _.isString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variableType is a hard-coded string that identifies what type the element is. It's used by the RecordedActions component which displays recorded code and it uses the variableType to determine if it should be el.text("String") or el.text(123) etc...

@mykola-mokhnach
Copy link
Contributor

Any chance to fix Travis run?

@dpgraham
Copy link
Contributor Author

Unfortunately there's a Travis problem that breaks the Electron build. I'm leaving it there in the hopes that eventually Travis gets fixed.

@dpgraham dpgraham merged commit f2c7ad4 into master Nov 21, 2017
@dpgraham dpgraham deleted the dpgraham-highlight-searched-elements branch November 21, 2017 07:58
dpgraham added a commit that referenced this pull request Nov 21, 2017
When user selects an element in the 'Search Elements' modal it highlights it on the screenshot.
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.

2 participants