Skip to content

feat(kibana-data-discovery): replace title props and wrap EuiButtonIcon with EuiToolTip#271481

Merged
weronikaolejniczak merged 5 commits into
elastic:mainfrom
weronikaolejniczak:feat/replace-title-kibana-data-discovery
May 29, 2026
Merged

feat(kibana-data-discovery): replace title props and wrap EuiButtonIcon with EuiToolTip#271481
weronikaolejniczak merged 5 commits into
elastic:mainfrom
weronikaolejniczak:feat/replace-title-kibana-data-discovery

Conversation

@weronikaolejniczak
Copy link
Copy Markdown
Contributor

@weronikaolejniczak weronikaolejniczak commented May 27, 2026

Relates to elastic/eui#9566

Important

These changes should be carefully tested visually by each code owner. Wrapping with EuiToolTip instead of passing title leads to another DOM node and can potentially break the layout. In such cases, I would appreciate committing appropriate fixes to this PR directly, I cannot possibly setup and run all Kibana functionalities to fix every regression.

This PR:

  • wraps ALL EuiButtonIcon with EuiToolTip, the content is the same as aria-label, any title passed to EuiButtonIcon is removed,
  • changes several title cases (not truncation related) to use EuiToolTip instead (if applicable).

@weronikaolejniczak weronikaolejniczak added backport:skip This PR does not require backporting EUI release_note:skip Skip the PR/issue when compiling release notes labels May 27, 2026
@weronikaolejniczak
Copy link
Copy Markdown
Contributor Author

buildkite test this

@alexwizp alexwizp added backport:version Backport to applied version labels v9.5.0 v9.4.2 and removed backport:skip This PR does not require backporting labels May 28, 2026
@weronikaolejniczak
Copy link
Copy Markdown
Contributor Author

buildkite test this

@weronikaolejniczak
Copy link
Copy Markdown
Contributor Author

buildkite test this

@weronikaolejniczak weronikaolejniczak marked this pull request as ready for review May 28, 2026 15:08
@weronikaolejniczak weronikaolejniczak requested review from a team as code owners May 28, 2026 15:08
@infra-vault-gh-plugin-prod
Copy link
Copy Markdown

Pinging @elastic/eui-team (EUI)

@akowalska622 akowalska622 self-requested a review May 28, 2026 15:40
Copy link
Copy Markdown
Contributor

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Only a test change on our code owned related to the changes to the unifiedTabs.

Copy link
Copy Markdown
Contributor

@akowalska622 akowalska622 left a comment

Choose a reason for hiding this comment

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

LGTM, leaving one nit and a question

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code changes LGTM for this file, but a side question to @AlexGPlay - do we still need this file? It seems like we migrated fully to EUI split button and looks like a dead code.

It's only imported by the deprecated TopNavMenuItem, but no caller uses it, maybe worth adding some cleanup issue to the backlog

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess first the TopNavMenuItem needs to be cleaned up and after that we can remove the @kbn/split-button package 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree it's dependent, but what I meant is to track it now (if it's good to go soon), otherwise we forget 😁

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, yes! that definitely sounds like a good idea 😁

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image

Comment on lines -173 to -176
await retry.try(async () => {
// click on the tab navigation left button 10 times to make the first tab visible
for (let i = 0; i < 10; i++) {
await testSubjects.click('unifiedTabs_tabsBar_scrollLeftBtn');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this deletion intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alexwizp?

Apparently, this was causing the FTRs to fail.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image Image Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good ✅

Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A tooltip obscures the cell menu, I've added a change suggestion below

Image

…ts/doc_viewer_table/table_cell_value.tsx

Co-authored-by: Ania Kowalska <63072419+akowalska622@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Scout Lane #9 - stateful-classic / default / local-stateful-classic - Saved query menu — CRUD (Discover) - save, load, update, save-as-new, delete via the popover

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
alertingVTwo 879.0KB 879.3KB +298.0B
cloudSecurityPosture 616.0KB 616.3KB +298.0B
dataViewFieldEditor 100.3KB 100.8KB +535.0B
dataViewManagement 145.6KB 146.0KB +437.0B
discover 1.9MB 1.9MB +575.0B
esql 921.6KB 921.9KB +298.0B
esqlDataGrid 156.0KB 156.3KB +298.0B
lens 2.1MB 2.1MB +114.0B
osquery 1.3MB 1.3MB +298.0B
securitySolution 12.1MB 12.1MB +298.0B
slo 1.2MB 1.2MB +298.0B
unifiedDocViewer 685.8KB 685.9KB +73.0B
workflowsManagement 2.5MB 2.5MB +298.0B
total +4.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
navigation 10.7KB 10.8KB +57.0B

History

@weronikaolejniczak weronikaolejniczak merged commit 5993118 into elastic:main May 29, 2026
33 checks passed
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 9.4

https://github.com/elastic/kibana/actions/runs/26631540508

@kibanamachine
Copy link
Copy Markdown
Contributor

💔 All backports failed

Status Branch Result
9.4 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 271481

Questions ?

Please refer to the Backport tool documentation

@miguel-sanchez-elastic
Copy link
Copy Markdown

Hi @weronikaolejniczak , I have seen that #270161 was closed but I tested main and I see the browser tooltips and not the new ones.

Maybe we were doing something custom in our end and it wasn't covered.

Do you suggest that you have another look or I can get someone from my team to have a look into it, it is fine either way.

For reference, our experience is in Discover, when running TS metrics-*

image

@akowalska622
Copy link
Copy Markdown
Contributor

akowalska622 commented May 29, 2026

The icons from the screenshot are specific for the metrics profile, they weren't covered in this PR I believe

@weronikaolejniczak
Copy link
Copy Markdown
Contributor Author

weronikaolejniczak commented May 29, 2026

@miguel-sanchez-elastic hey, Miguel 👋🏻 sorry, I think while breaking down the large PR I mistakenly added the "Closes" keyword to this one. The change needed lives in @kbn/shared-ux-button-toolbar so it's separate to this PR. I re-opened the task. Thank you for the vigilance 🙏🏻

@alexwizp
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
9.4

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

Questions ?

Please refer to the Backport tool documentation

@miguel-sanchez-elastic
Copy link
Copy Markdown

No problem @weronikaolejniczak !

Also I noticed the following in Discover FROM logs-:

image

dej611 pushed a commit to dej611/kibana that referenced this pull request May 29, 2026
…on with EuiToolTip (elastic#271481)

Closes elastic#270161

Relates to elastic/eui#9566

> [!IMPORTANT]
> These changes **should be carefully tested visually by each code
owner.** Wrapping with `EuiToolTip` instead of passing `title` leads to
another DOM node and can potentially break the layout. In such cases, I
would appreciate committing appropriate fixes to this PR directly, I
cannot possibly setup and run all Kibana functionalities to fix every
regression.

This PR:

- wraps ALL `EuiButtonIcon` with `EuiToolTip`, the content is the same
as `aria-label`, any `title` passed to `EuiButtonIcon` is removed,
- changes several `title` cases (not truncation related) to use
`EuiToolTip` instead (if applicable).

---------

Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Co-authored-by: Ania Kowalska <63072419+akowalska622@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels EUI release_note:skip Skip the PR/issue when compiling release notes v9.4.2 v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants