-
Notifications
You must be signed in to change notification settings - Fork 837
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
[EuiDataGrid][EuiSelectable] Add data-test attributes for more explicit E2E testing #5643
Conversation
- Now that it no longer has a unique className, it isn't easy for E2E tests to hook into it
- to make the transition/check for consuming E2E tests that check for selection state to use a data-attribute since we recently switched from aria-selected to aria-checked
data-test-selected={isChecked} // Whether the item is checked/selected | ||
aria-checked={role === 'option' ? isChecked : undefined} // Whether the item is "checked" |
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.
I quickly tested this on local with a random option set to { role: 'test' }
and this data attribute is actually needed for that use case, because it 100% always reflects the visual checked icon/state of the item whereas aria-checked
will no longer be present if role
is not option
.
TBF I can't really imagine a scenario where a consumer sets a custom role and still wants it to behave like an option, but we allow it in code, so 🤷♀️
CHANGELOG.md
Outdated
@@ -1,6 +1,6 @@ | |||
## [`main`](https://github.com/elastic/eui/tree/main) | |||
|
|||
No public interface changes since `48.1.0`. | |||
- Added two new `data-test` attributes to `EuiDataGrid` and `EuiSelectable` for more explicit E2E testing ([#5643](https://github.com/elastic/eui/pull/5643)) |
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.
Should this go under a **Bug fixes**
header if we want this to be a patch release?
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.
I think it's ok to just make it a patch during release. They're test-only changes.
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.
Snapshot changes LGTM
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5643/ |
Summary
While fixing and updating several Kibana CI tests in elastic/kibana#125023, it became clear that we should add a few more
data-test
attributes to a few of our components to make several E2E selectors more deliberate.This will hopefully be either a very small backport or patch release that we can then add to our Kibana upgrade.
Checklist