[EuiTourStep] `className` consistency with EuiPopover + performance/cleanups #7497

cee-chen · 2024-01-31T19:36:18Z

Summary

As always, I recommend following along by commit!

Breaking prop<->DOM changes

Applies EuiTourStep classNames to the anchor element instead of to the popover panel
- This change was made to be consistent with EuiPopover behavior (consumers cannot otherwise apply styles to the anchor since we deprecated the anchorClassName prop).
- Existing usages should be converted to panelClassName instead
Applies EuiTourStep's style prop to the anchor element instead of to the popover panel
- Also for popover consistency. Existing usages should be converted to panelStyle instead.
- See also [EuiTour] Fix positioning calculation issues caused by width style overrides #5897 (comment) for previous discussion/confusion about this prop

Tech debt

Converts all Enzyme tests for EuiTourStep to RTL and switches snapshots to assertions
Converts several inline CSS styles to their logical property equivalents
Refactors code heavily for performance; adds missing memoization and breaks out several conditional inline JSX to React.memo'd subcomponents which optimizes/reduces rerenders.
- Additionally, smaller subcomponents makes it easier to reason about individual bits of code/logic.

QA

https://eui.elastic.co/pr_7497/#/display/tour - confirm no regressions in UI or UX compared to production

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile - Note: appears to be somewhat buggy already in prod
- Checked in Chrome, Safari, Edge, and Firefox
  - [ ] Checked for accessibility including keyboard-only and screenreader modes** - EuiTour does not appear to be inherently accessible in production
Docs site QA - N/A - refactor
Code quality checklist
- Added or updated jest ~~and cypress tests~~
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 - N/A

- consumers who want to add a className to the popover panel should use `panelClassName` instead

- consumers who want to add styles to the popover panel should use `panelStyle` instead + fix/clean up extended EuiPopover props typing(s) to allow `style` prop

+ replace snapshots with explicit assertions

- grab EuiBeacon too while we're here

- split up into `memo`'d internal subcomponents (most performant option), which also has the side benefit of making code a little easier to reason about

- bah humbug

kibanamachine · 2024-01-31T20:05:51Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7497/
Storybook: https://eui.elastic.co/pr_7497/storybook

elasticmachine · 2024-01-31T20:05:54Z

💚 Build Succeeded

Buildkite Build
Commit: 85e5abe

github-actions · 2024-01-31T21:05:24Z

This PR contains breaking changes. The opener of this pull request is asked to perform the following due diligence steps below, to assist EUI in our next Kibana upgrade:

If this PR contains prop/API changes:
- Search through Kibana for <EuiComponent usages (example search)
- In the PR description or in a PR comment, include a count or list with the number of component usages in Kibana that will need to be updated (if that amount is "none", include that information as well)
~~If this PR contains CSS changes:~~

Log | Bot Usage

cee-chen · 2024-01-31T21:18:35Z

Checked all usages of <EuiTourStep> in Kibana and did not find any that were passing custom classNames or styles. 🎉

tkajtoch · 2024-02-02T16:42:16Z

src/components/tour/tour_step.test.tsx

@@ -7,7 +7,6 @@
 */

 import React from 'react';
-import { mount } from 'enzyme';


tkajtoch · 2024-02-02T16:59:14Z

src/components/tour/_tour_footer.tsx

+    const customFooterAction = useMemo(() => {
+      if (!footerAction) return null;
+
+      return Array.isArray(footerAction) ? (


I think using a regular if statement would be cleaner here, but I have no strong preference!

I'm a huge sucker for early returns 😅 I like that they mean the rest of the code has one less set of indentations!

tkajtoch · 2024-02-02T17:01:54Z

src/components/tour/tour_step.tsx

    );
  }

  const [anchorNode, setAnchorNode] = useState<HTMLElement | null>(null);
  const [popoverPosition, setPopoverPosition] = useState<EuiPopoverPosition>();

-  const onPositionChange = (position: EuiPopoverPosition) => {
+  const onPositionChange = useCallback((position: EuiPopoverPosition) => {


tkajtoch

LGTM! The changes look and work great 🎉

Feel free to ignore my comment about the if statement if you prefer to leave it as is!

cee-chen · 2024-02-02T17:14:53Z

Thanks a million Tomasz! Hope my comment about the early return makes sense, you'll definitely see me continue to do that in code if I can get away with it :shifty_eyes:

`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <jon@elastic.co>

`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([elastic#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([elastic#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([elastic#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([elastic#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([elastic#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([elastic#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([elastic#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([elastic#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <jon@elastic.co>

cee-chen added 7 commits January 31, 2024 09:56

Fix className DOM location to match EuiPopover

4f0c2ce

- consumers who want to add a className to the popover panel should use `panelClassName` instead

Fix style DOM location to match EuiPopover

43241a6

- consumers who want to add styles to the popover panel should use `panelStyle` instead + fix/clean up extended EuiPopover props typing(s) to allow `style` prop

[tests] Convert Enzyme to RTL

13f7ac2

+ replace snapshots with explicit assertions

[css] Convert inline styles to logical property equivalents

0f1971d

- grab EuiBeacon too while we're here

[Perf] Memoize & clean up code

049e269

- split up into `memo`'d internal subcomponents (most performant option), which also has the side benefit of making code a little easier to reason about

TS fixes

f0b0603

- bah humbug

changelog

85e5abe

cee-chen added the breaking change label Jan 31, 2024

cee-chen marked this pull request as ready for review January 31, 2024 21:24

cee-chen requested a review from a team as a code owner January 31, 2024 21:24

tkajtoch self-requested a review February 2, 2024 16:32

tkajtoch reviewed Feb 2, 2024

View reviewed changes

src/components/tour/tour_step.test.tsx

@@ -7,7 +7,6 @@

*/

import React from 'react';

import { mount } from 'enzyme';

Copy link

Member

tkajtoch Feb 2, 2024

Choose a reason for hiding this comment

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

🎉

tkajtoch reviewed Feb 2, 2024

View reviewed changes

tkajtoch approved these changes Feb 2, 2024

View reviewed changes

cee-chen merged commit ff19255 into elastic:main Feb 2, 2024
10 checks passed

cee-chen deleted the tour-step/className branch February 2, 2024 17:16

cee-chen mentioned this pull request Feb 5, 2024

Upgrade EUI to v93.0.0 elastic/kibana#176246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiTourStep] `className` consistency with EuiPopover + performance/cleanups #7497

[EuiTourStep] `className` consistency with EuiPopover + performance/cleanups #7497

cee-chen commented Jan 31, 2024 •

edited

Loading

kibanamachine commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 •

edited by cee-chen

Loading

cee-chen commented Jan 31, 2024

tkajtoch Feb 2, 2024

tkajtoch Feb 2, 2024

cee-chen Feb 2, 2024

tkajtoch Feb 2, 2024

tkajtoch left a comment

cee-chen commented Feb 2, 2024

[EuiTourStep] className consistency with EuiPopover + performance/cleanups #7497

[EuiTourStep] className consistency with EuiPopover + performance/cleanups #7497

Conversation

cee-chen commented Jan 31, 2024 • edited Loading

Summary

Breaking prop<->DOM changes

Tech debt

QA

General checklist

kibanamachine commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

💚 Build Succeeded

github-actions bot commented Jan 31, 2024 • edited by cee-chen Loading

cee-chen commented Jan 31, 2024

tkajtoch Feb 2, 2024

Choose a reason for hiding this comment

tkajtoch Feb 2, 2024

Choose a reason for hiding this comment

cee-chen Feb 2, 2024

Choose a reason for hiding this comment

tkajtoch Feb 2, 2024

Choose a reason for hiding this comment

tkajtoch left a comment

Choose a reason for hiding this comment

cee-chen commented Feb 2, 2024

[EuiTourStep] `className` consistency with EuiPopover + performance/cleanups #7497

[EuiTourStep] `className` consistency with EuiPopover + performance/cleanups #7497

cee-chen commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited by cee-chen

Loading