Skip to content

Conversation

@myasonik
Copy link
Contributor

@myasonik myasonik commented Apr 2, 2020

Summary

If EuiSelectable is also searchable it becomes an ARIA combobox. Largely, that means moving most of the aria-* props from the <ul> and onto the <input /> or surrounding <div>.

Largely this follows the 1.2 combobox spec with just a few fall back properties from 1.0 and 1.1.

Question!

Is this a breaking change?
EuiSelectableSearch and EuiSelectableList had prop changes but neither of those is really intended as a publicly consumable component individually though technically code be. The props for EuiSelectable have not changed.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11

  • Checked in Firefox
  • Props have proper autodocs
    - [ ] Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
    - [ ] A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@cchaos cchaos self-requested a review April 3, 2020 17:43
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl thompsongl self-requested a review April 9, 2020 14:41
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.

Is this a breaking change?

Yes. EuiSelectableList is part of the public API and now has 2 extra required props. Same with EuiSelectableSearch and its 1 new required prop. Whether they should be part of the public API is debatable, but removing them would also be a breaking change 😄

I noticed that the list in the 'Searchable' example no longer scrolls when operated with the keyboard. Is this intentional?

@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor Author

@thompsongl fixed!

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.

Looking good 👍
Functionality remains unchanged

@thompsongl thompsongl added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Apr 13, 2020
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.

Just realized that this is going into a feature branch, so no CL entry is necessary.

LGTM; tested locally

@myasonik
Copy link
Contributor Author

@cchaos did you still want to review this?

@kibanamachine
Copy link

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

@myasonik myasonik merged commit a0ef625 into elastic:feature/selectable-a11y Apr 15, 2020
@myasonik myasonik deleted the selectable-a11y/searchable branch April 15, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants