-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[atoms] use css locators in dom.js #13430
Conversation
8a12f9e
to
ffda382
Compare
ffda382
to
ceab5b8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #13430 +/- ##
=======================================
Coverage 58.48% 58.48%
=======================================
Files 86 86
Lines 5270 5270
Branches 220 220
=======================================
Hits 3082 3082
Misses 1968 1968
Partials 220 220 ☔ View full report in Codecov by Sentry. |
@twalpole I know you did some work on a separate isDisplayed atom for Capybara. Did you do something similar here? |
@titusfortner I just went through all the code and removed all the stuff that was supporting legacy browsers - which took it down to 4.7k - lib/capybara/selenium/atoms/isDisplayed.min.js - unminified 16.1k https://github.com/teamcapybara/capybara/blob/master/lib/capybara/selenium/atoms/src/isDisplayed.js. I did not specifically swap from xpath to css because of the added scoping functionality xpath provides, but it likely did involve getting rid of wicked-good-xpath since every modern browser supports it natively without needing a JS polyfill. |
":dom", | ||
":errors", | ||
":json", | ||
":useragent", | ||
":xpath", | ||
"//third_party/js/wgxpath", |
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.
So, do we need to depend on wicked good xpath here?
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.
Yes, this is still needed here for legacy browsers to have XPath locator support.
This PR does only remove the need for wicked good xpath for is-displayed atom.
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.
Which legacy browsers require it? IE 11?
We've also discussed moving from closure to Typescript for these. |
Is it possible to run TypeScript in IE 11 and old Android versions? |
CI Failure Feedback(Checks updated until commit d68c0fb)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
Wouldn't we transpile and minify it the same (ish) way closure does? |
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.
Thank you, @joerg1985!
Description
Using xpaths will need to transfer the complete
wicked-good-xpath
, this can easily be replaced by a css locator.Motivation and Context
This will reduce the size of the
isDisplayed.js
from ~43kb to ~17kb.Types of changes
Checklist