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

[Controls] Use EUI Selectable for Field search #151231

Merged
merged 9 commits into from
Feb 24, 2023
Merged

[Controls] Use EUI Selectable for Field search #151231

merged 9 commits into from
Feb 24, 2023

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Feb 14, 2023 •
edited
Loading

Summary

Fixes #151197

Before

before

After

after

@ThomThomson ThomThomson added release_note:enhancement Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. v8.8.0 labels Feb 14, 2023
@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@ThomThomson ThomThomson marked this pull request as ready for review February 21, 2023 21:27
@ThomThomson ThomThomson requested review from a team as code owners February 21, 2023 21:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson ThomThomson mentioned this pull request Feb 23, 2023
Merged
andreadelrio
andreadelrio requested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@andreadelrio andreadelrio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems that the field token is not aligned inside the Selectable Item.
image

Suggest adding vertical-align: middle to the EuiToken. Utility class eui-alignMiddle will do this too.

@ThomThomson
Copy link
Contributor Author

@andreadelrio good call, I added the utility class to center the items - It looks much better!
image

Heenawter
Heenawter approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Tested locally + code review - this works so much nicer, and the code is cleaner to boot 🎉 Great improvement! Left one tiny design comment + one other nit - otherwise, LGTM.

src/plugins/presentation_util/public/components/field_picker/field_picker.tsx
Comment on lines +108 to +109
isVirtualized: true,
showIcons: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is often a scrollbar, I think that bordered: true looks a bit nicer?

EuiSelectableBorder

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on Hannah's suggestion, a border would be a nice addition

Copy link
Contributor Author

@ThomThomson ThomThomson Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I felt that the border looked a bit too noisy, but after seeing the screenshots - and knowing that y'all are down for the border - I'll add it!

src/plugins/controls/public/control_group/editor/control_editor.tsx Outdated
{fieldRegistry && (
<EuiFormRow label={ControlGroupStrings.manageControl.getFieldTitle()}>
<FieldPicker
filterPredicate={(field: DataViewField) => Boolean(fieldRegistry?.[field.name])}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since you added the fieldRegistry && ... catch, could this be changed to:

Suggested change
filterPredicate={(field: DataViewField) => Boolean(fieldRegistry?.[field.name])}
filterPredicate={(field: DataViewField) => Boolean(fieldRegistry.[field.name])}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, committed this!

@andreadelrio andreadelrio self-requested a review February 23, 2023 21:40
andreadelrio
andreadelrio approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Tested locally. Design changes LGTM. As I mentioned in another comment, a border would look nice.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 179.2KB 179.2KB -1.0B
presentationUtil 130.9KB 129.0KB -1.9KB
total -1.9KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ThomThomson ThomThomson merged commit eb9cc11 into elastic:main Feb 24, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 24, 2023
@Heenawter Heenawter mentioned this pull request Feb 28, 2023
Closed
@Heenawter Heenawter mentioned this pull request Apr 20, 2023
Merged
@Heenawter
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Heenawter pushed a commit to Heenawter/kibana that referenced this pull request Apr 20, 2023
@ThomThomson
[Controls] Use EUI Selectable for Field search (elastic#151231)
b8a43f4 
## Summary
Replaces Control field selection list with EUISelectable.

(cherry picked from commit eb9cc11)

# Conflicts:
#	src/plugins/controls/public/control_group/editor/control_editor.tsx
@Heenawter Heenawter mentioned this pull request Apr 20, 2023
Closed
Heenawter added a commit that referenced this pull request Apr 20, 2023
@Heenawter @ThomThomson
[8.7] [Controls] Use EUI Selectable for Field search (#151231) (#155454)
cc16f95 
Closes #155452

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Controls] Use EUI Selectable for Field search
(#151231)](#151231)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Devon
Thomson","email":"devon.thomson@elastic.co"},"sourceCommit":{"committedDate":"2023-02-24T15:39:15Z","message":"[Controls]
Use EUI Selectable for Field search (#151231)\n\n## Summary\r\nReplaces
Control field selection list with
EUISelectable.","sha":"eb9cc11a7c81073a75fb3e6ec33e61796cc40aff","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","Feature:Dashboard","Feature:Input
Control","Team:Presentation","loe:days","impact:low","backport:skip","v8.8.0"],"number":151231,"url":"https://github.com/elastic/kibana/pull/151231","mergeCommit":{"message":"[Controls]
Use EUI Selectable for Field search (#151231)\n\n## Summary\r\nReplaces
Control field selection list with
EUISelectable.","sha":"eb9cc11a7c81073a75fb3e6ec33e61796cc40aff"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/151231","number":151231,"mergeCommit":{"message":"[Controls]
Use EUI Selectable for Field search (#151231)\n\n## Summary\r\nReplaces
Control field selection list with
EUISelectable.","sha":"eb9cc11a7c81073a75fb3e6ec33e61796cc40aff"}}]}]
BACKPORT-->

---------

Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
rylnd added a commit that referenced this pull request Apr 21, 2023
@rylnd
Merge branch '8.7' into risk_score_api_8.7
c70c7ea 
* 8.7: (93 commits)
  [8.7] [Controls] Use EUI Selectable for Field search (#151231) (#155454)
  [8.7] [Synthetics] Fix  performance breakdown link from error details page (#155393) (#155427)
  [8.7] [DOCS] Remove or move book-scoped attributes (#155210) (#155426)
  [8.7] [Synthetics] add default email recovery message (#154862) (#155418)
  [8.7] [Uptime] Add both both ip filters for view host in uptime location for host and monitor (#155382) (#155399)
  [8.7] Setup Node.js environment before instrumenting Kibana with APM. (#155063) (#155300)
  [8.7] [Discover] Address react warnings for legacy table (#154579) (#155345)
  [8.7] [Fleet] Fix logs useless rerender (#155305) (#155310)
  [8.7] [kbn-failed-test-reporter-cli] truncate report message to fix github api call failure (#155141) (#155286)
  [8.7][APM] Fleet migration support for bundled APM package (#153159)  (#155281)
  [8.7] [Enterprise Search] Fix Connector scheduling show week information correctly (#155191) (#155227)
  [8.7] [Synthetics] Fix pending count in case of location filtering (#155200) (#155225)
  [8.7] [Controls] Add Expensive Queries Fallback (#155082) (#155189)
  [8.7] [data view field editor] Runtime field code editor - move state out of controller (#155107) (#155150)
  [8.7] [FullStory] Update snippet (#153570) (#155138)
  [8.7] [Security Solution][Exceptions] - Fix exception operator logic when mapping conflict (#155071) (#155094)
  [DOCS] Adds 8.7.1 release notes (#154844)
  [8.7] Sync bundled packages with Package Storage (#155042)
  [APM] plugin description (#154811)
  Update api.asciidoc (#155021)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreadelrio andreadelrio andreadelrio approved these changes

@Heenawter Heenawter Heenawter approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.7.1 v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Controls] Virtualize Field List
6 participants
@ThomThomson @elasticmachine @kibana-ci @Heenawter @andreadelrio @kibanamachine