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

Support multiple space-separated values within a single element's data-test-subj attribute #1587

Merged

Conversation

cjcenizal
Copy link
Contributor

I've tested this in the context of Kibana, and this change doesn't seem to break any tests.

@cjcenizal cjcenizal added the testing Issues or PRs that only affect tests - will not need changelog entries label Feb 21, 2019
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

👍 Makes sense to me

@chandlerprall
Copy link
Contributor

Instead of always-on, how about passing a matcher to the function?

findTestSubject(comp, 'pagination page-button-1') // '=' default matcher
findTestSubject(comp, 'page-button-1', '~=') // whitespace-separated value
findTestSubject(comp, 'page-button-', '^=') // must start with

This way we can support all of the attribute selectors https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors#Syntax

@cjcenizal
Copy link
Contributor Author

@chandlerprall I think that's a great idea! How do you feel about ~= being the default? It seems like an enhancement to plain old = without any drawbacks. It also seems like a better DX, because I think there are a good number of people who would need to look the matcher syntax up to use it. 🖐

@chandlerprall
Copy link
Contributor

@cjcenizal that sounds good to me

@cjcenizal
Copy link
Contributor Author

@chandlerprall @thompsongl Updated with your feedback and added tests. Could you take another look please?

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks! A couple minor things

CHANGELOG.md Outdated
**Bug fixes**

- Fixed several bugs with `EuiRange` and `EuiDualRange` including sizing of inputs, tick placement, and the handling of invalid values ([#1580](https://github.com/elastic/eui/pull/1580))
>>>>>>> edf0b85abf0668db2fc7f3c282a077fcc2d6f121
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge remnants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops!

@@ -2,11 +2,12 @@
* Find node which matches a specific test subject selector. Returns ReactWrappers around DOM element,
* https://github.com/airbnb/enzyme/tree/master/docs/api/ReactWrapper.
* Common use cases include calling simulate or getDOMNode on the returned ReactWrapper.
*
* The ~= matcher looks for the value in space-separated list, allowing support for multiple data-test-subj
* values on a single element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a link to the attribute selector spec or similar to give a complete view of what matcher options are available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

@cjcenizal
Copy link
Contributor Author

Thanks, peeps. Ready for another look.

@cjcenizal cjcenizal merged commit 7ce11d3 into elastic:master Mar 1, 2019
@cjcenizal cjcenizal deleted the feature/multiple-test-subject-selectors branch March 1, 2019 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issues or PRs that only affect tests - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants