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

[EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable #5387

Merged
merged 48 commits into from
Feb 11, 2022
Merged

[EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable #5387

merged 48 commits into from
Feb 11, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Nov 17, 2021 •
edited
Loading

Summary

Closes #3735 and closes #4165 by rebuilding the EuiFilterGroup demo to use EuiSelectable
Also converts FieldValueSelectionFilter in EuiSearchBar to use EuiSelectable

Before:
Screen Shot 2021-11-17 at 11 25 04 AM

After:
Screen Shot 2021-11-17 at 11 27 15 AM

This also raises the question of whether EuiFilterSelectItem is even needed anymore. Should it be marked for deprecation?
To be done in #2841

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles

  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress 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_5387/

@cchaos
Copy link
Contributor

cchaos commented Nov 17, 2021

Quick reminder that EuiSearchBar/EuiInMemoryTable uses the filter component to auto-create their filter UI. You may want to update there as well.

@thompsongl
Copy link
Contributor Author

Quick reminder that EuiSearchBar/EuiInMemoryTable uses the filter component to auto-create their filter UI. You may want to update there as well.

Setting this back to draft while I look in to EuiSearchBar filters

@thompsongl thompsongl marked this pull request as draft November 17, 2021 22:48
@thompsongl thompsongl changed the title [EuiFilterGroup] Refactor demo with EuiSelectable [EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable Nov 22, 2021
@kibanamachine
Copy link

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

@thompsongl thompsongl marked this pull request as ready for review November 22, 2021 18:43
@kibanamachine
Copy link

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

@thompsongl thompsongl requested a review from cchaos December 1, 2021 20:40
@kibanamachine
Copy link

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

cchaos
cchaos reviewed Dec 3, 2021
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.

@thompsongl Can you also add some docs around the new props of EuiSelectable? I think it's important to tackle these as we add them even though it's not directly part of EuiFilterGroup.

src/components/selectable/selectable_list/selectable_list.tsx Outdated Show resolved Hide resolved
@thompsongl thompsongl mentioned this pull request Dec 6, 2021
Closed
24 tasks
@kibanamachine
Copy link

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

@thompsongl thompsongl requested a review from cchaos February 7, 2022 15:57
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

cchaos
cchaos reviewed Feb 9, 2022
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.

Do we have a plan for creating a real EuiFilterGroup that just takes a list of items? Like pulling FieldValueSelectionFilter out to a real component?

src-docs/src/views/filter_group/filter_group_multi.js Outdated Show resolved Hide resolved
src-docs/src/views/selectable/selectable_example.js Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor Author

Do we have a plan for creating a real EuiFilterGroup that just takes a list of items? Like pulling FieldValueSelectionFilter out to a real component?

I don't think so. I can create an issue if it's something we want to add.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Feb 9, 2022

I can create an issue if it's something we want to add.

Yes, please!

cchaos
cchaos reviewed Feb 9, 2022
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.

I know this isn't exactly part of this PR, but can we remove the logic that re-orders the items based on selection?
Screen Shot 2022-02-09 at 13 53 00 PM
Screen Shot 2022-02-09 at 13 52 50 PM

@thompsongl
Copy link
Contributor Author

I know this isn't exactly part of this PR, but can we remove the logic that re-orders the items based on selection?

After looking at this, I think it should be a separate discussion and not tacked on to this PR.
For longer lists, the current behavior seems better, and I think it constitutes a breaking change if we don't make it configurable. I can open an issue or discussion for changing the behavior.

@thompsongl thompsongl requested a review from cchaos February 10, 2022 15:40
@kibanamachine
Copy link

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

cchaos
cchaos reviewed Feb 10, 2022
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.

That's fine if we want to re-evaluate the re-ordering maybe at the same time we turn this into a real component.

src-docs/src/views/selectable/selectable_example.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

This was referenced Feb 11, 2022
Closed
Closed
@thompsongl thompsongl requested a review from cchaos February 11, 2022 15:16
cchaos
cchaos approved these changes Feb 11, 2022
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.

Thanks! Looking forward to seeing this get turned into a real component too!

@thompsongl thompsongl enabled auto-merge (squash) February 11, 2022 15:26
@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 90a003c into elastic:main Feb 11, 2022
@cee-chen cee-chen mentioned this pull request Feb 16, 2022
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

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@thompsongl @kibanamachine @cchaos @breehall @cee-chen