[EuiBasicTable] Implement `nameTooltip` API in `columns` prop #8273

acstll · 2025-01-14T14:01:31Z

Summary

This PR implements the feature request from #7963

It adds a nameTooltip prop for columns in the EuiBasicTable, in order to easily render a tooltip in a column header.

Regarding the actual API, I left a comment in the original issue with a question; and a comment in the source.

Before

name: (
  <EuiToolTip content="Their mascot is the Octokitty">
    <>
      Github{' '}
      <EuiIcon
        size="s"
        color="subdued"
        type="questionInCircle"
        className="eui-alignTop"
      />
    </>
  </EuiToolTip>
),

After

name: 'GibHub',
nameTooltip: {
  content: 'Their mascot is the Octokitty',
},

QA

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
- Added documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
- ~~Checked Code Sandbox works for any docs examples~~
Code quality checklist
- Added or updated jest ~~and cypress tests~~
- Updated visual regression tests (⚠️ there's the issue with the font rendering, so they might need an update once the issue is resolved)
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)~~
Designer checklist
- If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

mgadewoll · 2025-01-21T09:06:04Z

packages/eui/src/components/basic_table/basic_table.tsx

@@ -1280,6 +1282,9 @@ export class EuiBasicTable<T extends object = any> extends Component<
      sortable,
      footer,
      mobileOptions,
+      // do not pass it down, we don't want it in the DOM
+      // (is this the right way to do this?)


Yes, this is right. When spreading ...rest on a component or element we want to ensure unexpected props are excluded, so restructuring them as you did here ensures that 👍

mgadewoll · 2025-01-21T09:17:04Z

packages/eui/src/components/table/table_header_cell.test.tsx

+        </EuiTableHeaderCell>
+      );
+
+      expect(container.firstChild).toMatchSnapshot();


I know this file had already a lot of snapshots, but generally it would be nicer to assert that the element is available instead of using a snapshot, imho.

Edit: Here also a recent conversation on this

ah thanks for the link! will remove all the unnecessary snapshots then (you noticed I kinda added everything)

mgadewoll · 2025-01-21T09:37:29Z

packages/eui/src/components/table/table_header_cell.tsx

+      size="m"
+      color="subdued"
+      iconProps={tooltip.iconProps}
+      {...tooltip.tooltipProps}


Yeah, I'm not the biggest fan of having tooltip.tooltipProps, that seems convoluted 😅
If the prop on the header cell is tooltip and the main purpose is adding a tooltip I'd assume to have the props directly on the type (similar to EuiIconTip)

Otherwise I'd assume the prop to be called something else (like maybe iconTip 🤔) but that would not match the API in place for e.g. EuiIconTip so we should better align.

I understand your point, I'll revise the API to try and polish it

@mgadewoll what do you think about this:

we make the prop in EuiTableHeaderCell be iconTip, and it just spreads into EuiIconTip

the current test in EuiTableHeaderCell will check it renders correctly

we keep the API as it is, columns.nameTooltip for EuiBasicTable

we add a unit test for EuiBasicTable to check the API

and we do the "normalisation" in basic_table.tsx while rendering the header cells:

const { icon, tooltipProps, ...iconTipProps } = nameTooltip; const sharedProps = { width, iconTip: { type: icon ?? 'questionInCircle', ...tooltipProps, ...iconTipProps, }, description, mobileOptions, align: columnAlign, append: this.renderCopyChar(index), };

yes or nah? 🙂

or we just make it columns.nameIconTip as well and no normalisation is needed, super easy and transparent… 🤔

Hmm, yeah I think this might be a good idea.
My initial worry was that we generally call tooltips tooltip in the APIs and "tooltip" is generally a bit more understandable what's happening than "iconTip". But this is not actually a tooltip on the cell or even the cell name/content but it adds an actual EuiIconTip, so I think this does make sense 👍

On EuiTableHeaderCell it should be iconTipProps then considering our common usage of such an API for passing props to components.

mgadewoll · 2025-01-21T09:43:06Z

packages/eui/src/components/table/table_header_cell.tsx

+          size="m"
+          color="subdued"
+          iconProps={tooltip.iconProps}
+          {...{ position: 'top', ...tooltip.tooltipProps }}


Suggestion: Why combine the props as object on the fly? It's doing the same but - at least to me - this seems more clean since we're spreading props on the component anyway:

position="top" {...tooltip.tooltipProps}

much cleaner indeed, thank you

mgadewoll · 2025-01-21T09:54:10Z

packages/eui/src/components/table/table_header_cell.stories.tsx

@@ -66,3 +66,14 @@ export const Playground: Story = {
    onSort: false,
  },
 };
+
+export const Tooltip: Story = {


Let's have this open by default so VRT is more useful.

We can use play on the story object to define interactions, e.g. something like this:

play: async ({ step }) => { await step('show tooltip on hover', async () => { await waitFor(async () => { await fireEvent.mouseOver(document.querySelector('.euiToolTipAnchor')!); }); }); },

Additionally we'll need to change the selector in which the VRT image is taken to ensure the tooltip is included:

parameters: { loki: { chromeSelector: LOKI_SELECTORS.portal, }, },

lovely, I will include this (I was actually not sure because it seemed to me the important part was the icon position, and the tooltip is a different component — also I didn't know how to do this haha)

I tried with different logic (I found in other stories) in the play function but couldn't get it to work — I'll keep trying!

mgadewoll · 2025-01-21T09:56:03Z

packages/eui/src/components/table/table_header_cell.stories.tsx

+    tooltip: {
+      content: 'tooltip content',
+    },
+  },


For stories showcasing specific behavior, we don't include all props but only relevant ones as all others are already shown by the "playground" story.

You can use parameters.controls.include to define props that should be included.

mgadewoll · 2025-01-21T09:57:09Z

packages/eui/src/components/basic_table/table_types.ts

+  /**
+   * Allows adding an icon with a tooltip displayed next to the name
+   */
+  nameTooltip?: EuiTableHeaderCellTooltip;


Considering we add this option, I'd vote for updating an EuiBasicTable story to include this prop for one column.

mgadewoll · 2025-01-21T10:06:54Z

packages/eui/src-docs/src/views/tables/sorting/sorting.tsx

-      </EuiToolTip>
-    ),
+    name: 'Date of Birth',
+    nameTooltip: {


Considering that EuiInMemoryTable is a wrapper around EuiBasicTable, we should also include an example in docs/storybook. I think it's fine to just add it to one column in an existing example to showcase.

will do, nice!

mgadewoll · 2025-01-21T10:16:23Z

packages/eui/changelogs/upcoming/8273.md

@@ -0,0 +1 @@
+- Updated `EuiBasicTable` to allow adding a tooltip to the header cells, with a new `nameTooltip` key in the `columns` prop


We updated EuiTableHeaderCell too 🙂

Suggestion:

- Updated table components to support adding tooltips to header cells - Added `tooltip` prop on `EuiTableHeaderCell` - Added `columns.nameTooltip` on `EuiBasicTable`

that's much better 👌

mgadewoll · 2025-01-21T10:24:15Z

packages/eui/src/components/table/table_header_cell.test.tsx

+        </EuiTableHeaderCell>
+      );
+
+      expect(container).toMatchSnapshot();


I'd think this snapshot is not needed?

mgadewoll · 2025-01-21T10:28:37Z

packages/eui/src/components/table/table_header_cell.test.tsx

+        </EuiTableHeaderCell>
+      );
+
+      expect(container.firstChild).toMatchSnapshot();


Maybe we could instead assert DOM order based on checking the nodes and their position? 🤔

I will give it a try

mgadewoll · 2025-01-21T10:28:47Z

packages/eui/src/components/table/table_header_cell.test.tsx

+      expect(container.firstChild).toMatchSnapshot();
+    });
+
+    it('handles expected props', async () => {


Imho, this is the more appropriate test: We assert that the props create an expected DOM output (based on icon and tooltip shown).

Maybe we can remove the first added test in favor of this one.

Edit: Maybe we can make the description a bit more precise though, something like renders an icon with tooltip

perfect, I agree — regarding the description I was trying to match the overall tone, I can definitely rephrase it

kibanamachine · 2025-01-23T10:48:51Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_8273/
Storybook: https://eui.elastic.co/pr_8273/storybook
New docs: https://eui.elastic.co/pr_8273/new-docs

elasticmachine · 2025-01-23T10:49:55Z

💚 Build Succeeded

Buildkite Build
Commit: 4751ea5

History

💚 Build #3351 succeeded 0f096ad
💚 Build #3339 succeeded e977b8a
💚 Build #3335 succeeded e36fa2b
💚 Build #3319 succeeded d433f01

cc @acstll

acstll mentioned this pull request Jan 14, 2025

[EuiBasicTable] add nameTooltip API #7963

Open

acstll force-pushed the basic-table-nametooltip-7963 branch from 1f08961 to dcf3d6f Compare January 20, 2025 14:07

acstll marked this pull request as ready for review January 20, 2025 14:14

acstll requested a review from a team as a code owner January 20, 2025 14:14

mgadewoll reviewed Jan 21, 2025

View reviewed changes

JasonStoltz assigned mgadewoll and acstll and unassigned mgadewoll Jan 21, 2025

acstll added 16 commits January 22, 2025 19:13

[EuiBasicTable] Implement nameTooltip API in columns prop

c976d4f

[EuiBasicTable] Add unit tests for nameTooltip API

3c9f088

Update changelog

9af31c4

Restore stories

7818378

[EuiTableHeaderCell] Add story for tooltip

c069667

[EuiTableHeaderCell] Make tooltip position default to 'top'

64c49e8

Update docs for Tables

a64274a

[EuiTableHeaderCell] Add reference for VRT

90870fb

Improve comment

66734fc

Refactor EuiIconTipProps and export

a063c50

Update changelog

194683e

Refactor API to use iconTip for transparency

34f4cfa

Update docs

b7b988e

Fix type import for EuiIconTip stories

78cb22c

Fix wrong prop name in changelog

14ad751

Add parameters.loki in EuiTableHeaderCell stories (WIP)

0f096ad

acstll force-pushed the basic-table-nametooltip-7963 branch from 570bb48 to 0f096ad Compare January 22, 2025 18:14

acstll requested a review from mgadewoll January 22, 2025 18:19

acstll marked this pull request as draft January 23, 2025 08:19

Fix prop name in story

4751ea5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiBasicTable] Implement `nameTooltip` API in `columns` prop #8273

[EuiBasicTable] Implement `nameTooltip` API in `columns` prop #8273

acstll commented Jan 14, 2025 •

edited

Loading

mgadewoll Jan 21, 2025

mgadewoll Jan 21, 2025 •

edited

Loading

acstll Jan 21, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

acstll Jan 22, 2025

acstll Jan 22, 2025

mgadewoll Jan 22, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

acstll Jan 22, 2025

mgadewoll Jan 21, 2025

mgadewoll Jan 21, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

mgadewoll Jan 21, 2025

mgadewoll Jan 21, 2025

acstll Jan 21, 2025

mgadewoll Jan 21, 2025 •

edited

Loading

acstll Jan 21, 2025

kibanamachine commented Jan 23, 2025

elasticmachine commented Jan 23, 2025

		@@ -0,0 +1 @@
		- Updated `EuiBasicTable` to allow adding a tooltip to the header cells, with a new `nameTooltip` key in the `columns` prop

[EuiBasicTable] Implement nameTooltip API in columns prop #8273

Are you sure you want to change the base?

[EuiBasicTable] Implement nameTooltip API in columns prop #8273

Conversation

acstll commented Jan 14, 2025 • edited Loading

Summary

Before

After

QA

General checklist

Choose a reason for hiding this comment

mgadewoll Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgadewoll Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jan 23, 2025

elasticmachine commented Jan 23, 2025

💚 Build Succeeded

History

[EuiBasicTable] Implement `nameTooltip` API in `columns` prop #8273

[EuiBasicTable] Implement `nameTooltip` API in `columns` prop #8273

acstll commented Jan 14, 2025 •

edited

Loading

mgadewoll Jan 21, 2025 •

edited

Loading

mgadewoll Jan 21, 2025 •

edited

Loading