Skip to content

[EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable#5387

Merged
thompsongl merged 48 commits intoelastic:mainfrom
thompsongl:3735-filtergroup
Feb 11, 2022
Merged

[EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable#5387
thompsongl merged 48 commits intoelastic:mainfrom
thompsongl:3735-filtergroup

Conversation

@thompsongl
Copy link
Copy Markdown
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

@thompsongl thompsongl added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Nov 17, 2021
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Nov 17, 2021

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

@cchaos
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

thompsongl 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.

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 removed the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Nov 22, 2021
@thompsongl thompsongl changed the title [EuiFilterGroup] Refactor demo with EuiSelectable [EuiFilterGroup][FieldValueSelectionFilter] Use EuiSelectable Nov 22, 2021
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Nov 22, 2021

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
Copy Markdown

kibanamachine commented Nov 29, 2021

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
Copy Markdown

kibanamachine commented Dec 1, 2021

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

cchaos
cchaos reviewed Dec 3, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@thompsongl thompsongl mentioned this pull request Dec 6, 2021
Closed
24 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Dec 7, 2021

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
Copy Markdown

kibanamachine commented Feb 7, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Feb 7, 2022

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

cchaos
cchaos reviewed Feb 9, 2022
View reviewed changes
Copy link
Copy Markdown
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?

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented Feb 9, 2022

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
Copy Markdown

kibanamachine commented Feb 9, 2022

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

@cchaos
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

thompsongl commented Feb 10, 2022

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
Copy Markdown

kibanamachine commented Feb 10, 2022

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

cchaos
cchaos reviewed Feb 10, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Feb 10, 2022

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
Copy Markdown
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
Copy Markdown

kibanamachine commented Feb 11, 2022

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

2 more reviewers

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

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiFilterGroup] Multi-select example not functional [EuiFilterGroup] Rebuild multi-select demo using EuiSelectable

5 participants

@thompsongl @kibanamachine @cchaos @breehall @cee-chen