[Shared UX] Migrate popover from presentation to shared ux package #128665

rshen91 · 2022-03-28T17:05:00Z

Summary

This PR migrates the popover from the items directory in presentation_util to the shared ux package.

The ToolbarButton is refactored to accept a specified iconSide prop to accommodate controls such as control_group_container.tsx where the icon is on the right.
ts-ignore comment added to bypass the error:

info [bazel] packages/kbn-shared-ux-components/src/toolbar/popovers/popover.tsx:35:6 - error TS2322: Type '{ children: ReactNode; onClick?: MouseEventHandler | undefined; color?: string | undefined; style?: CSSProperties | undefined; ... 276 more ...; closePopover: () => void; }' is not assignable to type 'IntrinsicClassAttributes'.
| info [bazel] Types of property 'css' are incompatible.
| info [bazel] Type 'InterpolationWithTheme' is not assignable to type 'Interpolation'.
| info [bazel] Type 'ArrayInterpolation' is not assignable to type 'Interpolation'.
| info [bazel] Type 'ArrayInterpolation' is not assignable to type 'ArrayInterpolation'.
| info [bazel] The types returned by 'pop()' are incompatible between these types.
| info [bazel] Type 'Interpolation' is not assignable to type 'Interpolation'.
| info [bazel] Type 'ArrayInterpolation' is not assignable to type 'Interpolation'.
| info [bazel]
| info [bazel] 35 <EuiPopover {...{ isOpen, button, closePopover }} {...popover}>
| info [bazel] ~~~~~~~~~~
| info [bazel]
| info [bazel]
| info [bazel] Found 1 error in packages/kbn-shared-ux-components/src/toolbar/popovers/popover.tsx:35

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…pover

majagrubic

This looks pretty sound to me, but @clintandrewhall if you could have an additional look, would be appreciated.

clintandrewhall · 2022-03-31T18:31:23Z

packages/kbn-shared-ux-components/src/toolbar/buttons/primary/primary.tsx

@@ -9,14 +9,16 @@
 import React from 'react';
 import { EuiButton } from '@elastic/eui';
 import { EuiButtonPropsForButton } from '@elastic/eui/src/components/button/button';
+import { ButtonContentIconSide } from '@elastic/eui/src/components/button/button_content';


Suggested change

import { ButtonContentIconSide } from '@elastic/eui/src/components/button/button_content';

import { EuiButtonDisplayProps } from '@elastic/eui/src/components/button/button_content';

clintandrewhall · 2022-03-31T18:31:58Z

packages/kbn-shared-ux-components/src/toolbar/buttons/primary/primary.tsx


 export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' | 'iconType'> {
  label: string;
+  iconSide?: ButtonContentIconSide;


Suggested change

iconSide?: ButtonContentIconSide;

clintandrewhall · 2022-03-31T18:33:20Z

packages/kbn-shared-ux-components/src/toolbar/buttons/primary/primary.tsx

@@ -9,14 +9,16 @@
 import React from 'react';
 import { EuiButton } from '@elastic/eui';
 import { EuiButtonPropsForButton } from '@elastic/eui/src/components/button/button';
+import { ButtonContentIconSide } from '@elastic/eui/src/components/button/button_content';

 export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' | 'iconType'> {


Suggested change

export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' | 'iconType'> {

export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' | 'iconType' | 'iconSide'> {

clintandrewhall · 2022-03-31T18:35:21Z

packages/kbn-shared-ux-components/src/toolbar/buttons/primary/primary.tsx

  return (
-    <EuiButton size="m" color="primary" fill={true} {...rest}>
+    <EuiButton size="m" color="primary" fill={true} iconSide={iconSide} {...rest}>


I don't think this is necessary, but it it is:

Suggested change

<EuiButton size="m" color="primary" fill={true} iconSide={iconSide} {...rest}>

<EuiButton size="m" color="primary" fill={true} {...{ iconSide, ...rest }>

clintandrewhall · 2022-03-31T18:38:11Z

packages/kbn-shared-ux-components/src/toolbar/popovers/popover.tsx

+  const button = <ToolbarButton onClick={onButtonClick} {...{ label, iconSide, iconType }} />;
+
+  return (
+    // @ts-ignore - Types of property csss are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.


@thompsongl Any suggestions on this particular error?

Suggested change

// @ts-ignore - Types of property csss are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

// @ts-expect-error - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

I don't see any errors here locally, and switching to @ts-expect-error introduces an error

type_check also passes

Is there somewhere else the error presents?

My guess is this was a leftover from some previous version of EUI. I removed it now.

Ok, by removing it I get the following error:

Type '{ children: ReactNode; onClick?: MouseEventHandler<HTMLDivElement> \| undefined; color?: string \| undefined; style?: CSSProperties \| undefined; ... 276 more ...; closePopover: () => void; }' is not assignable to type 'IntrinsicClassAttributes<EuiPopover>'. -- | info [bazel] Types of property 'css' are incompatible. | info [bazel] Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'. | info [bazel] Type 'ArrayInterpolation<undefined>' is not assignable to type 'Interpolation<Theme>'. | info [bazel] Type 'ArrayInterpolation<undefined>' is not assignable to type 'ArrayInterpolation<Theme>'. | info [bazel] The types returned by 'pop()' are incompatible between these types. | info [bazel] Type 'Interpolation<undefined>' is not assignable to type 'Interpolation<Theme>'. | info [bazel] Type 'ArrayInterpolation<undefined>' is not assignable to type 'Interpolation<Theme>'. | info [bazel] | info [bazel] 35 <EuiPopover {...{ isOpen, button, closePopover }} {...popover}>

Ah, just saw you were talking about ts-ignore vs ts-expect-error rather than the actual type error not being present :D

To clarify, I don't get any errors after removing the TS comment entirely. bazel build and type_check both pass, and I get no errors in my IDE. Where do you see that error?

Oh I see it now on bootstrap. That error is from a type mismatch between emotion@10 and emotion@11. Storybook uses emotion@10 and EUI, Kibana, etc. use emotion@11. This is more likely a bazel config problem (perhaps from how it builds with kbn-storybook package?) more so than an actual type error. Thinking on how to resolve.

Adding the following to kbn-shared-ux-components/tsconfig.json compilerOptions resolves the error:

"paths": { "@emotion/core": [ "node_modules/@emotion/react" ] }

Also, the error will be resolved for us in the next release of storybook: https://github.com/storybookjs/storybook/releases/tag/v6.5.0-alpha.31

clintandrewhall

We need to take the fix provided by @thompsongl for the Typescript incompatibility.

clintandrewhall · 2022-04-01T19:42:48Z

packages/kbn-shared-ux-components/src/toolbar/popovers/popover.tsx

+  const button = <ToolbarButton onClick={onButtonClick} {...{ label, iconSide, iconType }} />;
+
+  return (
+    // @ts-ignore - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.


We should rarely-- if ever-- use @ts-ignore, because if the error is resolved elsewhere, the ignore will remain. If we use @ts-expect-error, changes that resolve the error will prompt the resolver to remove this comment.

Suggested change

// @ts-ignore - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

// @ts-expect-error - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

clintandrewhall · 2022-04-01T19:43:36Z

@elasticmachine merge upstream

kibanamachine · 2022-04-01T19:43:48Z

expected head sha didn’t match current head ref.

…nto migrate-popover

majagrubic · 2022-04-04T11:37:49Z

@elasticmachine merge upstream

majagrubic · 2022-04-05T07:15:47Z

Synced offline with @spalger about the ts problem, this is the conclusion:

I think the underlying issue is that while we expose the tsconfig.base.json file to bazel packages when they're built we don't expose the typings/* directory, so the "@emotion/core": ["typings/@emotion"], directive in tsconfig.base.json is silently ignored by TypeScript in the package build (because typings/@emotion can't be resolved).
I think it might be time to move the base tsconfigs to a package so that we can have all of their dependencies available when we're using them in another package. In the meantime we should probably stick with a // @ts-ignore

majagrubic · 2022-04-06T12:09:34Z

@elasticmachine merge upstream

clintandrewhall

Please complete my two documentation requests before committing. Approving to unblock.

clintandrewhall · 2022-04-07T17:05:39Z

packages/kbn-shared-ux-components/src/toolbar/popovers/popover.mdx

+date: 2022-03-28
+---
+
+> This documentation is in-progress.


We need to complete this before committing.

clintandrewhall · 2022-04-07T17:06:10Z

packages/kbn-shared-ux-components/src/toolbar/popovers/popover.tsx

+  const button = <ToolbarButton onClick={onButtonClick} {...{ label, iconSide, iconType }} />;
+
+  return (
+    // @ts-ignore


This needs a comment before committing.

majagrubic · 2022-04-10T09:54:05Z

@elasticmachine merge upstream

kibana-ci · 2022-04-10T11:27:10Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataViewEditor`	153	154	+1
`maps`	863	864	+1
`spaces`	264	265	+1
total			+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/shared-ux-components`	4	6	+2

Async chunks

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

id	before	after	diff
`dataViewEditor`	154.0KB	154.5KB	+538.0B
`maps`	2.7MB	2.7MB	+531.0B
`spaces`	268.6KB	269.1KB	+531.0B
total			+1.6KB

Page load bundle

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

id	before	after	diff
`dataViewEditor`	10.6KB	10.6KB	+1.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/shared-ux-components`	32	34	+2

History

💚 Build #36730 succeeded aada066
💛 Build #36440 was flaky 8050ebb
💚 Build #36082 succeeded 623ea04
💔 Build #35843 failed fa4f66d
💚 Build #35643 succeeded d8912a3
💔 Build #35553 failed 4986765

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

cc @rshen91

rshen91 added 8 commits March 28, 2022 10:49

initial commit

42e381a

remove styling from popover

d298dea

update test

6ecfe69

update

c139d8f

Merge branch 'main' into migrate-popover

55cf6cc

remove primary

fe3f301

Merge remote-tracking branch 'upstream/main' into migrate-popover

6f7282b

Merge remote-tracking branch 'origin/migrate-popover' into migrate-po…

0fc0ede

…pover

rshen91 added 8.3 candidate Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:skip This commit does not require backporting labels Mar 28, 2022

rshen91 added 11 commits March 28, 2022 16:22

updates

6fa55c9

Merge remote-tracking branch 'upstream/main' into migrate-popover

7845b3f

allow iconSide

e1ba381

Merge remote-tracking branch 'upstream/main' into migrate-popover

491e18c

remove unused

b7b5a2d

update

5a241b7

Merge remote-tracking branch 'upstream/main' into migrate-popover

b6a86e8

update with ts ignore

8045fa0

update test

5cf3f1e

Merge remote-tracking branch 'upstream/main' into migrate-popover

f3da9d5

missed add from lib test update

596b1cb

rshen91 self-assigned this Mar 30, 2022

Merge branch 'main' into migrate-popover

17c1d99

rshen91 mentioned this pull request Mar 30, 2022

[Meta] Transition Components to SharedUX From React Plugin #122961

Open

15 tasks

rshen91 added 3 commits March 30, 2022 09:56

update docs

725ff2c

Merge remote-tracking branch 'origin/migrate-popover' into migrate-po…

bfa1a18

…pover

Merge remote-tracking branch 'upstream/main' into migrate-popover

7544aee

rshen91 marked this pull request as ready for review March 30, 2022 16:00

rshen91 added the v8.3.0 label Mar 30, 2022

rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label Mar 30, 2022

rshen91 requested review from majagrubic and sebelga March 30, 2022 16:05

majagrubic approved these changes Mar 31, 2022

View reviewed changes

clintandrewhall reviewed Mar 31, 2022

View reviewed changes

clintandrewhall requested changes Mar 31, 2022

View reviewed changes

clintandrewhall reviewed Mar 31, 2022

View reviewed changes

Maja Grubic added 2 commits April 1, 2022 16:15

Addressing PR comments

0f23e1e

Reintroduce ts-ignore

4986765

clintandrewhall requested changes Apr 1, 2022

View reviewed changes

Merge branch 'main' into migrate-popover

d8912a3

Maja Grubic added 2 commits April 4, 2022 13:29

Apply fix for ts-error

f96b0ac

Merge branch 'migrate-popover' of https://github.com/rshen91/kibana i…

9dc98ea

…nto migrate-popover

Merge branch 'main' into migrate-popover

fa4f66d

Reintroduce ts-ignore

623ea04

majagrubic requested a review from clintandrewhall April 5, 2022 12:05

kibanamachine and others added 2 commits April 6, 2022 08:09

Merge branch 'main' into migrate-popover

8050ebb

Merge branch 'main' into migrate-popover

aada066

clintandrewhall approved these changes Apr 7, 2022

View reviewed changes

Updating comments

d44c921

Merge branch 'main' into migrate-popover

bc1b7c2

majagrubic merged commit 2894e40 into elastic:main Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Shared UX] Migrate popover from presentation to shared ux package #128665

[Shared UX] Migrate popover from presentation to shared ux package #128665

rshen91 commented Mar 28, 2022 •

edited

Loading

majagrubic left a comment

clintandrewhall Mar 31, 2022

clintandrewhall Mar 31, 2022

clintandrewhall Mar 31, 2022

clintandrewhall Mar 31, 2022

clintandrewhall Mar 31, 2022 •

edited

Loading

thompsongl Mar 31, 2022 •

edited

Loading

thompsongl Mar 31, 2022

majagrubic Apr 1, 2022

majagrubic Apr 1, 2022

majagrubic Apr 1, 2022

thompsongl Apr 1, 2022

thompsongl Apr 1, 2022

thompsongl Apr 1, 2022

clintandrewhall left a comment

clintandrewhall Apr 1, 2022

clintandrewhall commented Apr 1, 2022

kibanamachine commented Apr 1, 2022

majagrubic commented Apr 4, 2022

majagrubic commented Apr 5, 2022

majagrubic commented Apr 6, 2022

clintandrewhall left a comment

clintandrewhall Apr 7, 2022

clintandrewhall Apr 7, 2022

majagrubic commented Apr 10, 2022

kibana-ci commented Apr 10, 2022

API count

	import { ButtonContentIconSide } from '@elastic/eui/src/components/button/button_content';
	import { EuiButtonDisplayProps } from '@elastic/eui/src/components/button/button_content';

	export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' \| 'iconType'> {
	export interface Props extends Pick<EuiButtonPropsForButton, 'onClick' \| 'iconType' \| 'iconSide'> {

	<EuiButton size="m" color="primary" fill={true} iconSide={iconSide} {...rest}>
	<EuiButton size="m" color="primary" fill={true} {...{ iconSide, ...rest }>

	// @ts-ignore - Types of property csss are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.
	// @ts-expect-error - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

	// @ts-ignore - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.
	// @ts-expect-error - Types of property css are incompatible Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<Theme>'.

[Shared UX] Migrate popover from presentation to shared ux package #128665

[Shared UX] Migrate popover from presentation to shared ux package #128665

Conversation

rshen91 commented Mar 28, 2022 • edited Loading

Summary

Checklist

majagrubic left a 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

clintandrewhall Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Mar 31, 2022 • 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

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall commented Apr 1, 2022

kibanamachine commented Apr 1, 2022

majagrubic commented Apr 4, 2022

majagrubic commented Apr 5, 2022

majagrubic commented Apr 6, 2022

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Apr 10, 2022

kibana-ci commented Apr 10, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

rshen91 commented Mar 28, 2022 •

edited

Loading

clintandrewhall Mar 31, 2022 •

edited

Loading

thompsongl Mar 31, 2022 •

edited

Loading