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

Assign EuiComboBox data-test-subj to the options list with a '-optionsList' suffix #1054

Merged
merged 6 commits into from
Jul 30, 2018

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 27, 2018

This is so you can find a specific combo box instance's options list. This wasn't previously possible because the options list is attached to the body element, not the combo box element.

@cjcenizal
Copy link
Contributor Author

Looks like the icon snapshots were removed in #1040, which is why I think they're showing up in this PR.

CC @snide @cchaos

CHANGELOG.md Outdated
@@ -8,6 +8,10 @@ No public interface changes since `3.2.1`.
- Added types for `EuiToast`, `EuiGlobalToastList`, and `EuiGlobalToastListItem` ([#1045](https://github.com/elastic/eui/pull/1045))
- Added a handful of third-party logos to `EuiIcon` ([#1033](https://github.com/elastic/eui/pull/1033))

**Breaking changes**

- `EuiComboBox` now applies the provided `data-test-subj` to its options list element with the suffix `-optionsList` so you can find a specific combo box instance's options list. This wasn't previously possible because the options list is attached to the body element, not the combo box element. This is a break from the previous `data-test-subj` which was simply `comboBoxOptionsList`. ([#1054](https://github.com/elastic/eui/pull/1054))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this as a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kibana has tests which rely on the data-test-subj="comboBoxOptionsList" selector, and they'll break once we intake this change.

Copy link
Contributor

@chandlerprall chandlerprall Jul 27, 2018

Choose a reason for hiding this comment

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

I tried to find that line in the PR, but couldn't (sorry). Can that attribute be left in place and just add the one for individual options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I'll make that change.

@cjcenizal
Copy link
Contributor Author

@chandlerprall Thanks, done.

@snide
Copy link
Contributor

snide commented Jul 27, 2018

@cjcenizal Thanks. I'll fix.

@snide
Copy link
Contributor

snide commented Jul 27, 2018

@cjcenizal err, I mean, thanks for fixing.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small changelog note.

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@ No public interface changes since `3.2.1`.

## [`3.2.1`](https://github.com/elastic/eui/tree/v3.2.1)

- `EuiComboBox` now applies the provided `data-test-subj` to its options list element with the suffix `-optionsList` so you can find a specific combo box instance's options list. This wasn't previously possible because the options list is attached to the body element, not the combo box element. This is in addition to the existing `data-test-subj="comboBoxOptionsList"`. ([#1054](https://github.com/elastic/eui/pull/1054))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be under master and labeled as a breaking change.

Copy link
Contributor

@chandlerprall chandlerprall Jul 27, 2018

Choose a reason for hiding this comment

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

not a breaking change, this is now only adding new attributes without removing the old one (but yes, under master)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review, inspected dom to view new data-test-subj attributes

@cjcenizal cjcenizal merged commit 35f77ba into elastic:master Jul 30, 2018
@cjcenizal cjcenizal deleted the combo-box-list-test branch July 30, 2018 21:37
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.

4 participants