Remove Sass `@euiToolTip` mixin usages by cee-chen · Pull Request #194652 · elastic/kibana

cee-chen · 2024-10-02T00:02:42Z

Summary

This PR is part of EUI's ongoing Emotion migration, and removes EuiToolTip Sass mixins that will shortly be removed/deprecated from EUI.

3 usages of tooltip style mixins have been replaced with its migrated Emotion/CSS-in-JS equivalent. This is a somewhat stopgap measure - if possible, we'd prefer consumers not reach directly into our internal/styles and use the baseline <EuiToolTip> component instead. That being said, I've QA'd (almost) all instances and have confirmed that the affected tooltips still look the same as before (see below inline comment threads for before/after screenshots).

Checklist

This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

- normally we don't recommend reaching directly into components for styles, but this is the easiest migration path forward currently

- Emotion styles come with font size/line height set, this unsets it to previous render

- normally we don't recommend reaching directly into components for styles, but this is the easiest migration path forward currently

…ith EUI Sass variables

cee-chen · 2024-10-02T02:04:03Z

src/plugins/chart_expressions/expression_xy/public/components/tooltip/tooltip.scss

@@ -1,15 +1,12 @@
 .detailedTooltip {
  pointer-events: none;
-  @include euiToolTipStyle('s');


Before After

cee-chen · 2024-10-02T02:05:41Z

x-pack/plugins/canvas/public/components/layout_annotations/layout_annotations.scss

 }

 .tooltipAnnotation {
-  @include euiToolTipStyle($size: 's');


Before After

cee-chen · 2024-10-02T02:07:38Z

x-pack/plugins/ml/public/application/components/chart_tooltip/_chart_tooltip.scss

@@ -1,19 +1,18 @@
 .mlChartTooltip {
-  @include euiToolTipStyle('s');


Before After

In the screenshot above the tooltip is now hiding the red marker on the chart.

elasticmachine · 2024-10-02T02:09:01Z

Pinging @elastic/eui-team (EUI)

cee-chen · 2024-10-02T02:09:53Z

src/plugins/vis_types/vislib/public/vislib/components/tooltip/_tooltip.scss

@@ -1,12 +1,15 @@
 .visTooltip,
 .visTooltip__sizingClone {
-  @include euiToolTipStyle('s');


@elastic/kibana-visualizations I'm not sure where this tooltip is being used or how to reproduce it/see it in the browser - I'm fairly sure this change should work without major issue, but please let me know if not.

Hi @cee-chen you can test it by turning on the visualization:visualize:legacyHeatmapChartsLibrary advanced setting and creating an Agg Based heatmap.

I've tested locally and I believe we need to bring all the properties there, because the color you have assigned are different then before.
Before:

After:

Unfortunately the code there is not React so you can't use the hooks for EUI.
Probably just copying the shadow/border/background/color is enough

Ah I hadn't realized the difference was that stark, I'll copy the shadeOrTint Sass exactly in that case. It's still fairly fragile however as a warning - If EuiToolTip's styles ever change in the future (there is talks of a new theme on the horizon) then you'll need to manually update your custom tooltip to match.

I know, but just consider that these tooltip and visualization editor will be remove in the future, so don't worry about that too much.
So please go on with and copy the EUI tooltip styles for now, thanks

@cee-chen I'm just waiting for this to approve, whenever you have it ready please ping me

kibana-ci · 2024-10-02T03:51:50Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: fdc2a17

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #12 / CaseDetailsLink renders the children instead of the detail name if provided
[job] [logs] Jest Tests #12 / ConnectorSelector should set the selected connector to none if the connector is not available

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1291	1425	+134
`expressionXY`	254	388	+134
`ml`	2040	2174	+134
total			+402

Async chunks

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

id	before	after	diff
`canvas`	1.1MB	1.2MB	+178.0KB
`expressionXY`	127.7KB	305.5KB	+177.8KB
`ml`	4.6MB	4.8MB	+178.1KB
`visTypeVislib`	371.6KB	371.1KB	-494.0B
total			+533.4KB

Page load bundle

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

id	before	after	diff
`expressionXY`	41.9KB	42.0KB	+111.0B
`ml`	74.4KB	74.4KB	+53.0B
total			+164.0B

History

💛 Build #238608 was flaky e90ba3d93b52a7e1f05b51b745820fc33cebd1e3

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

jgowdyelastic · 2024-10-02T08:52:43Z

x-pack/plugins/ml/public/application/components/chart_tooltip/chart_tooltip.tsx

 import TooltipTrigger from 'react-popper-tooltip';
-import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
+import { EuiFlexGroup, EuiFlexItem, useEuiMemoizedStyles } from '@elastic/eui';
+// @ts-ignore Kibana does not have types for eui/lib


Suggested change

// @ts-ignore Kibana does not have types for eui/lib

// @ts-expect-error Kibana does not have types for eui/lib

@ts-expect-error would probably be better here as it'll catch when these types are fixed.

jgowdyelastic · 2024-10-02T09:09:06Z

Tooltip placement in ML seems to be broken with this fix. They now overflow off the side of the screen.
It appears than previously tooltips would appear on the left or right of the cursor depending on whether the cursor is on the left of right of the screen.

Before

After

nreese

kibana-presentation changes LGTM
code review only

cee-chen · 2024-10-14T20:42:29Z

Hey y'all! Super apologies for dropping the ball on this PR for a bit. I ended up realizing that EUI unfortunately can't deprecate the @euiToolTip Sass mixin just yet as elastic-charts relies on it to render its tooltips. As such, this PR isn't needed in Kibana, so I'm going to go ahead and close it. I'll investigate another approach EUI can take that could play well with Sass or just vanilla CSS.

[Canvas] Replace Sass tooltip mixin with CSS-in-JS styles

2571731

- normally we don't recommend reaching directly into components for styles, but this is the easiest migration path forward currently

cee-chen added release_note:skip Skip the PR/issue when compiling release notes EUI v9.0.0 v8.16.0 backport:version Backport to applied version labels labels Oct 2, 2024

cee-chen added 6 commits October 1, 2024 17:19

[Canvas] Restore previous tooltip height

69c7b94

- Emotion styles come with font size/line height set, this unsets it to previous render

[Chart Expressions] Replace Sass mixin with CSS-in-JS styles

00fb747

- normally we don't recommend reaching directly into components for styles, but this is the easiest migration path forward currently

[Chart Expressions] Fix tooltip display

1eb042c

[ML] Replace Sass tooltip mixin with CSS-in-JS styles

fad06b1

- normally we don't recommend reaching directly into components for styles, but this is the easiest migration path forward currently

[ML] Improve tooltip display for smaller/responsive screens

7db5dd8

[Visualize Library] Replace vanilla CSS visTooltip EUI mixin with w…

fdc2a17

…ith EUI Sass variables

cee-chen mentioned this pull request Oct 2, 2024

Delete remaining component-specific mixins elastic/eui#8055

Merged

1 task

cee-chen force-pushed the eui/remove-tooltip-mixin branch from e90ba3d to fdc2a17 Compare October 2, 2024 02:02

cee-chen commented Oct 2, 2024

View reviewed changes

cee-chen marked this pull request as ready for review October 2, 2024 02:08

cee-chen requested review from a team as code owners October 2, 2024 02:08

cee-chen commented Oct 2, 2024

View reviewed changes

jgowdyelastic reviewed Oct 2, 2024

View reviewed changes

nreese approved these changes Oct 2, 2024

View reviewed changes

MichaelMarcialis approved these changes Oct 2, 2024

View reviewed changes

jgowdyelastic mentioned this pull request Oct 4, 2024

[ML] Replace custom chart tooltips with elastic chart tooltips #194920

Open

cee-chen closed this Oct 14, 2024

cee-chen deleted the eui/remove-tooltip-mixin branch October 14, 2024 20:42

		@@ -1,19 +1,18 @@
		.mlChartTooltip {
		@include euiToolTipStyle('s');

	// @ts-ignore Kibana does not have types for eui/lib
	// @ts-expect-error Kibana does not have types for eui/lib

Conversation

cee-chen commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markov00 Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Oct 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

jgowdyelastic Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nreese left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Oct 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cee-chen commented Oct 2, 2024 •

edited

Loading

markov00 Oct 4, 2024 •

edited

Loading

jgowdyelastic Oct 2, 2024 •

edited

Loading

jgowdyelastic commented Oct 2, 2024 •

edited

Loading

nreese left a comment •

edited

Loading