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

Issue/1625 implement element closest #1626

Merged
merged 4 commits into from
Nov 28, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Nov 27, 2019

Fixes #1625

Adds to the local jsdom patches the implementation of Element.closest() as seen at jsdom/jsdom#1555 (comment).

Even though jsdom itself has had that function added already, the jsdom-jscore fork we are using is not up-to-date to use it. Updating the fork is far from trivial so, resorting to a local patch instead.

To test:

  • Android Appium tests on CI should pass

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Android Appium tests on CI should pass

Android Appium tests on CI does pass ✅

Thank you @hypest !

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Minor comment (probably not a blocker). Also, is the gutenberg reference update related?

Comment on lines 159 to 160
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not super important, but I believe this should return null if no matching element is found. I don't think it's likely, but in theory, someone could check for element.closest( selector ) === null (instead of just checking that it's falsey). Currently, it'd return undefined.

Spec: https://dom.spec.whatwg.org/#dom-element-closest

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch @mkevins !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Done with 919526c.

@hypest
Copy link
Contributor Author

hypest commented Nov 27, 2019

is the gutenberg reference update related?

Not really. There are no Gutenberg side changes that are relevant. I included an update of the ref for making sure this fix does indeed fix the tests that only get broken if you update to a recent Gutenberg hash. Other than that, this can be considered a typical Gutenberg "bump" to master to bring in the latest.

Would you prefer to just leave out the Gutenberg hash update @mkevins , now that we know the tests pass?

@hypest
Copy link
Contributor Author

hypest commented Nov 27, 2019

Ready for another pass!

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

Verified this works as expected on the line where we were previously crashing when pasting with and without a <pre> tag

@cameronvoell
Copy link
Contributor

cameronvoell commented Nov 28, 2019

Ready for another pass!

Thanks for adding this @hypest

Since we already have three approvals, I think we can go ahead and merge, unless @mkevins has any other concerns on the newly added code?

@mkevins
Copy link
Contributor

mkevins commented Nov 28, 2019

is the gutenberg reference update related?

Not really. There are no Gutenberg side changes that are relevant. I included an update of the ref for making sure this fix does indeed fix the tests that only get broken if you update to a recent Gutenberg hash. Other than that, this can be considered a typical Gutenberg "bump" to master to bring in the latest.

Would you prefer to just leave out the Gutenberg hash update @mkevins , now that we know the tests pass?

No need I just wanted to make sure I wasn't missing anything. Thanks for the explanation, makes sense!

@mkevins
Copy link
Contributor

mkevins commented Nov 28, 2019

Ready for another pass!

Thanks for adding this @hypest

Since we already have three approvals, I think we can go ahead and merge, unless @mkevins has any other concerns on the newly added code?

No concerns here. LGTM! :shipit:

@cameronvoell cameronvoell merged commit ca75471 into develop Nov 28, 2019
@hypest hypest deleted the issue/1625-implement-element-closest branch November 28, 2019 09:21
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.

NoSuchElement in Android Appium test after updating to Gutenberg master
4 participants