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.

Sign up for GitHub

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

Jump to bottom

[EuiSelectable+EuiComboBox] recent regresion fixes #3784

Merged
merged 4 commits into from
Jul 22, 2020
Merged

[EuiSelectable+EuiComboBox] recent regresion fixes #3784

merged 4 commits into from
Jul 22, 2020

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Jul 22, 2020 •
edited
Loading

Summary

Fixes two regressions caused by the recent selectable-a11y branch.

  1. Adding space as a option for how to selection items removes the ability to have items with in them so reverting this change.
  2. With the new react-window library replacing react-virtualized, this in conjunction with EuiFormRow was causing an extra rerender on click of virtualized elements which broke clicking... Adding a check in EuiFormRow for if the thing needs to rerender fixes it.

Checklist

  • Checked in mobile
  • Checked in Firefox
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately
    - [ ] Checked Code Sandbox works for the any docs examples
    - [ ] Added or updated jest tests
    - [ ] Props have proper autodocs
    - [ ] Added documentation
    - [ ] Checked in IE11
    - [ ] Check against all themes for compatibility in both light and dark modes

chandlerprall
chandlerprall suggested changes Jul 22, 2020
View reviewed changes
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.

Changelog formatting nit-picks but otherwise this LGTM!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@myasonik @chandlerprall
Changelog tweaks
4894c22 
Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
@myasonik myasonik requested review from chandlerprall and cchaos July 22, 2020 17:59
cchaos
cchaos approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Tested for both original bugs which are both fixed! Thanks for the quick PR!

@kibanamachine
Copy link

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

chandlerprall
chandlerprall approved these changes Jul 22, 2020
View reviewed changes
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.

❤️

@thompsongl thompsongl merged commit a71aaf8 into elastic:master Jul 22, 2020
@thompsongl thompsongl mentioned this pull request Jul 23, 2020
Closed
@thompsongl thompsongl mentioned this pull request Jul 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@myasonik @kibanamachine @chandlerprall @cchaos @thompsongl