Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Feb 16, 2022

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

- 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
@cee-chen cee-chen requested a review from thompsongl February 16, 2022 16:58
Comment on lines +248 to 249
data-test-selected={isChecked} // Whether the item is checked/selected
aria-checked={role === 'option' ? isChecked : undefined} // Whether the item is "checked"
Copy link
Contributor Author

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
## [`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))
Copy link
Contributor Author

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?

Copy link
Contributor

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.

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.

Snapshot changes LGTM

Co-authored-by: Greg Thompson <[email protected]>
@cee-chen cee-chen enabled auto-merge (squash) February 16, 2022 17:18
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5643/

@cee-chen cee-chen merged commit 2364046 into elastic:main Feb 16, 2022
@cee-chen cee-chen deleted the data-attr-backport branch February 16, 2022 18:16
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.

3 participants