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.

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

[EuiComboBox] add support for option tooltips #7700

Merged

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Apr 18, 2024

Summary

This PR is the first part of updates for #7690

This PR updates EuiComboBox to support the two props toolTipContent and toolTipProps to be passed to combobox options to display a tooltip on hover/focus.

To ensure the tooltip can be opened on focus, the EuiToolTip was updated to a support controlled behavior via isOpen.

Screenshot 2024-04-18 at 23 19 43

QA

  • review and test EuiComboBox in storybook and docs and/or checkout this PR locally via gh pr checkout 7700
    • ensure the tooltip is applied as expected (on hover as well as on keyboard navigation or click)

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)

@mgadewoll mgadewoll force-pushed the feat/7690-combobox-option-tooltip-support branch from 623e956 to c580005 Compare April 18, 2024 21:55
@mgadewoll mgadewoll marked this pull request as ready for review April 18, 2024 22:23
@mgadewoll mgadewoll requested a review from a team as a code owner April 18, 2024 22:23
@cee-chen cee-chen self-requested a review April 22, 2024 00:28
@cee-chen
Copy link
Contributor

@mgadewoll Thanks so much with your patience waiting for my review on this, and for your amazing work with granular commits that made it super easy to follow!! My largest comments I'd like to see resolved are:

  1. Firefox behavior (must fix, also not sure if this is only affecting me?)
  2. Avoiding a new API for EuiToolTip /using a class ref method instead (open to pushback on this if you feel strongly, but I do also wonder if this ties into the FF bug above)
  3. Changing the anchor position default to left instead of bottom

Everything other comment is just nice to have. I'll skip reviewing the EuiSelectable version of this PR as well (assuming it has similar-ish comments to the above) until this PR is settled and merged in if that's cool!

@cee-chen cee-chen force-pushed the feat/7690-combobox-option-tooltip-support branch 2 times, most recently from 759f203 to 685209d Compare May 2, 2024 19:32
@mgadewoll
Copy link
Contributor Author

ℹ️ I added baseline VRT references for EuiComboBox. The tooltip story currently looks the same as the playground one, I want to update it to use interactions to show an opened tooltip state instead, but I'd suggest doing it in a separate PR as I'll need to check on the setup first.

@cee-chen
Copy link
Contributor

cee-chen commented May 3, 2024

I want to update it to use interactions to show an opened tooltip state instead, but I'd suggest doing it in a separate PR as I'll need to check on the setup first.

Yasss that would be amazing! I reiterate that you are the best!

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🚢 🎉 Thanks again for your awesome work on this Lene!!! My comments below are very optional lint type nits, feel free to ignore them and merge without, up to you

src/components/combo_box/combo_box.stories.tsx Outdated Show resolved Hide resolved
src/components/combo_box/combo_box.test.tsx Show resolved Hide resolved
@mgadewoll mgadewoll force-pushed the feat/7690-combobox-option-tooltip-support branch from 0b7cb47 to 9929c3e Compare May 3, 2024 16:51
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mgadewoll mgadewoll merged commit 079cce4 into elastic:main May 3, 2024
5 checks passed
@mgadewoll mgadewoll deleted the feat/7690-combobox-option-tooltip-support branch May 6, 2024 08:30
jbudz pushed a commit to elastic/kibana that referenced this pull request May 10, 2024
`v94.2.1-backport.0` ⏩ `v94.3.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v94.3.0`](https://github.com/elastic/eui/releases/v94.3.0)

- Updated `launch` glyph for `EuiIcon`
([#7670](elastic/eui#7670))
- Updated `EuiComboBox`'s `options` to support including tooltip details
for selectable options. Use `toolTipContent` to render tooltip
information, and `toolTipProps` to optionally customize the tooltip
rendering behavior ([#7700](elastic/eui#7700))
- Updated the following existing glyphs in `EuiIcon`:
([#7727](elastic/eui#7727))
  - `error` (now an outlined version instead of filled) 
  - `tokenMetricCounter`
  - `tokenMetricGauge` 
- Added the following new glyphs to `EuiIcon`:
([#7727](elastic/eui#7727))
  - `tokenDimension`
  - `clickLeft`
  - `clickRight`
  - `clockCounter`
  - `errorFilled` (the previous `error` glyph design)
  - `warningFilled`

**Bug fixes**

- Fixed a visual layout bug for `EuiComboBox` with `isLoading` in mobile
views ([#7700](elastic/eui#7700))
- Fixed missing styles on header cells of `EuiDataGrid` that prevented
content text alignment styles to apply
([#7720](elastic/eui#7720))
- Fixed `EuiFlexGroup` and `EuiFlexItem` `ref` prop typing to support
refs of the same type as the passed `component` type and allow
`displayName` to be defined for easy component naming when using
component wrappers like `styled()`
([#7724](elastic/eui#7724))

---

Most of the code changes you'll see in this PR are caused by the recent
EuiFlex* changes making it generic. This, unfortunately, is something
that `styled()` doesn't always like. I replaced the failing usages of
`styled(EuiFlexGroup)` and `styled(EuiFlexItem)` to use `component` and
other native EuiFlex* props, resulting in the same output but being
better typed.

We plan to add more props to EuiFlex* components giving developers
control over properties like `flex-grow` and `flex-shring`, and reducing
the need for writing any custom CSS when using these components. This
should reduce the number of `styled()` wrappers needed even further

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants