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

feat: add class name locator strategy where applicable #803

Merged
merged 1 commit into from
May 9, 2023

Conversation

eglitise
Copy link
Collaborator

@eglitise eglitise commented May 8, 2023

This PR adds the class name locator strategy in the 'Find By' table when selecting elements with unique class names.
Considering that this strategy is listed as the fastest for both iOS and Android, I think it is worth adding.
Android uses the class element property, whereas iOS uses type.

Example:
image

@jlipps
Copy link
Member

jlipps commented May 8, 2023

Before I review the code, probably worth having a conceptual discussion around this. The id/xpath/ax id selectors are presented as ways to identify this element. class name is not unique and therefore is not typically going to actually successfully find the element under consideration except by accident. I'm not sure if this PR already includes this, but I think we would only want to show the class name as a selector if we can determine that it's uniquely targeting the right element (i.e., there is only one element of that class on the page). Or that we should have a warning icon or something.

@eglitise
Copy link
Collaborator Author

eglitise commented May 9, 2023

@jlipps I fully agree. Conveniently, the existing code already had a uniqueness check for all the optimal locator strategies, so I did not need to change anything there :) The class name row is therefore only shown for the elements where it can be used as a unique identifier.

@eglitise eglitise force-pushed the add-class-name-locator-strategy branch from c07a21d to 1029e4a Compare May 9, 2023 06:14
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.

ok sounds great!

@eglitise eglitise merged commit 31c46f0 into appium:main May 9, 2023
@eglitise eglitise deleted the add-class-name-locator-strategy branch May 9, 2023 16:47
shiva-guntoju pushed a commit to shiva-guntoju/appium-inspector that referenced this pull request Feb 2, 2024
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.

2 participants