[shared-ux] Migrate toolbar button component #124372

rshen91 · 2022-02-02T16:33:53Z

Summary

This PR migrates the toolbar button component from the kibana-react folder to the shared-ux directory

Where toolbars similarly are used in the codebase:

Lens has a ToolbarPopover component that seems similar x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx
- used as LegendSettingsPopover in lens/public/shared_components/legend_settings_popover.tsx
- Lens has ToolbarPopover used in datatable_visualization in x-pack/plugins/lens/public/datatable_visualization/components/toolbar.tsx
- used in HeatmapToolbar in heatmap_visualization/toolbar_component.tsx
- used in PieToolbar in pie_visualization/toolbar.tsx
- used in GaugeToolbar in visualizations/gauge/toolbar_component/index.tsx
- AxisSettingsPopover in xy_visualization/xy_config_panel/axis_settings_popover.tsx
- VisualOptionsPopover in xy_visualization/xy_config_panel/visual_options_popover/index.tsx
SolutionToolbarPopover PresentationToolbar in src/plugins/presentation_util/public/components/solution_toolbar/solution_toolbar.tsx
- this might just need to be migrated over too e9a025b
- EditorMenu in dashboard has SolutionToolbarPopover in src/plugins/dashboard/public/application/top_nav/editor_menu.tsx

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

rshen91 · 2022-02-03T14:24:17Z

@elasticmachine merge upstream

rshen91 · 2022-02-07T14:13:19Z

@elasticmachine merge upstream

rshen91 · 2022-02-08T21:19:02Z

@elasticmachine merge upstream

…rate-toolbar-button

majagrubic · 2022-02-11T07:28:41Z

src/plugins/shared_ux/public/components/solution_toolbar/items/add_from_library.tsx

+  <SolutionToolbarButton
+    {...rest}
+    iconType="folderOpen"
+    onClick={onClick}


Why is onClick destructured explicitly and not with the rest?

I think what's happening is that the solution toolbar allows consumers to pass their own onClick handler. I'm looking at button.tsx with the props extending EUIButtonPropsForButton and seeing the following:

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

@majagrubic has a point: there's no need to destructure props on line 17 to extract onClick, because we're not doing anything with it. You could actually do:

<SolutionToolbarButton iconType="folderOpen" {...props} />

majagrubic · 2022-02-11T07:32:24Z

src/plugins/shared_ux/public/components/solution_toolbar/items/button.tsx

+  extends Pick<EuiButtonPropsForButton, 'onClick' | 'iconType' | 'iconSide' | 'className'> {
+  label: string;
+  primary?: boolean;
+  isDarkModeEnabled?: boolean;


is this being used anywhere?

Some of it is being used in the solution_toolbar.tsx file in the props. In the solution toolbar implementation, the primary styling is used consistently in how the toolbar is set up.

I'm tempted to keep this as is since the items directory comes directly from the presentation team. I'm thinking as I migrate more components over, more and more references will occur with parts of the items folder.

majagrubic · 2022-02-11T07:33:16Z

src/plugins/shared_ux/public/components/solution_toolbar/items/popover.tsx

+
+  return (
+    <EuiPopover
+      anchorPosition="downLeft"


should this be customizable or are we ok with it always being down & left?

I'm open to making this customizable - thoughts @clintandrewhall?

I would expose and default it.

It's already exposed on line 18... just need to use it here. Pretty sure downLeft is the EUI default, as well.

majagrubic · 2022-02-11T07:40:17Z

src/plugins/shared_ux/public/components/solution_toolbar/solution_toolbar.tsx

+      className={`solutionToolbar ${
+        isDarkModeEnabled ? 'solutionToolbar--dark' : 'solutionToolbar--light'
+      }`}
+      id={`kbnPresentationToolbar__solutionToolbar`}


Do we want to be adding ids here? I would say it needs to be passed as a prop then

17c81e7 for changes, let me know if I'm understanding correctly, thanks

clintandrewhall

This is a great start...! Keep up the good work. You might find that splitting this into smaller PRs allows you to focus on the individual components... but that's up to you.

clintandrewhall · 2022-02-14T17:56:10Z

src/plugins/shared_ux/public/components/solution_toolbar/index.ts

+ */
+
+export { SolutionToolbar } from './solution_toolbar';
+export * from './items';


nit: best practice is to explicitly name the exports.

Ok! Fixed in e27b041 thank you!

clintandrewhall · 2022-02-14T17:57:40Z

src/plugins/shared_ux/public/components/solution_toolbar/items/add_from_library.tsx

+  <SolutionToolbarButton
+    {...rest}
+    iconType="folderOpen"
+    onClick={onClick}


@majagrubic has a point: there's no need to destructure props on line 17 to extract onClick, because we're not doing anything with it. You could actually do:

<SolutionToolbarButton iconType="folderOpen" {...props} />

clintandrewhall · 2022-02-14T17:58:26Z

src/plugins/shared_ux/public/components/solution_toolbar/items/button.scss

@@ -0,0 +1,12 @@
+.solutionToolbarButton {


We should not be introducing any SASS to SharedUX, and it we must, we must document it here: #122594

I removed it in e27b041 thank you!

clintandrewhall · 2022-02-14T18:36:29Z

src/plugins/shared_ux/public/components/solution_toolbar/items/popover.tsx

+
+  return (
+    <EuiPopover
+      anchorPosition="downLeft"


It's already exposed on line 18... just need to use it here. Pretty sure downLeft is the EUI default, as well.

clintandrewhall · 2022-02-14T18:37:22Z

src/plugins/shared_ux/public/components/solution_toolbar/items/primary_button.tsx

+    primary={true}
+    className={`solutionToolbar__primaryButton ${
+      isDarkModeEnabled
+        ? 'solutionToolbar__primaryButton--dark'


I really want to see all of this SASS gone. :-/

Removed in 7fa17de - is this the right approach or should I document it in the #122594?

clintandrewhall · 2022-02-14T18:39:07Z

src/plugins/shared_ux/public/i18n/components.ts

+
+import { i18n } from '@kbn/i18n';
+
+export const ComponentStrings = {


This is a deprecated approach from my time on Presentation Team. See: #103013

Let me know if my changes in 7fa17de make sense thanks!

clintandrewhall · 2022-02-14T18:39:26Z

src/plugins/shared_ux/public/i18n/index.ts

+ * Side Public License, v 1.
+ */
+
+export * from './labs';


copy-pasta error?

Removed in e27b041 thanks!

clintandrewhall · 2022-02-14T18:39:49Z

src/plugins/shared_ux/public/i18n/labs.tsx

+import { FormattedMessage } from '@kbn/i18n-react';
+import { EuiCode } from '@elastic/eui';
+
+export const LabsStrings = {


we shouldn't be bringing labs strings in...?

Removed in e27b041 thanks!

rshen91 · 2022-02-15T13:58:59Z

@elasticmachine merge upstream

…rate-toolbar-button

cchaos

Quick question: Why do the Toolbar Button storybook examples not have any text in the buttons?

rshen91 · 2022-02-15T21:12:37Z

Quick question: Why do the Toolbar Button storybook examples not have any text in the buttons?

Good call let me add some in the examples thanks!

majagrubic

Codewise looks good, waiting for the design to approve

clintandrewhall · 2022-02-16T17:40:14Z

I'm working on an in-depth review. Please don't merge yet.

clintandrewhall

I have some comments in the works. Bear with me for a few minutes.

kibana-ci · 2022-02-16T20:56:05Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`sharedUX`	23	35	+12

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
`sharedUX`	0	4	+4

Async chunks

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

id	before	after	diff
`sharedUX`	8.5KB	9.4KB	+882.0B

Page load bundle

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

id	before	after	diff
`sharedUX`	3.8KB	4.6KB	+874.0B

Unknown metric groups

API count

id	before	after	diff
`sharedUX`	10	16	+6

async chunk count

id	before	after	diff
`sharedUX`	1	2	+1

ESLint disabled in files

id	before	after	diff
`sharedUX`	2	3	+1

Total ESLint disabled count

id	before	after	diff
`sharedUX`	3	4	+1

History

💔 Build #24750 failed f89d158
💚 Build #24370 succeeded 1c8b762
💔 Build #24350 failed 7fa17de
💔 Build #24261 failed be3306e
💔 Build #24245 failed 56bcc47

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

rshen91 · 2022-02-17T15:56:26Z

Closing in favor of splitting this PR into smaller PRs that are more targeted in scope thanks!

rshen91 mentioned this pull request Feb 2, 2022

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

Open

15 tasks

rshen91 added loe:medium Medium Level of Effort backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Feb 2, 2022

rshen91 mentioned this pull request Feb 3, 2022

[EUI toolbar button component][shared-ux] elastic/eui#5594

Closed

rshen91 changed the title ~~[shared-ux]migrate toolbar button component~~ [shared-ux] Migrate toolbar button component Feb 7, 2022

rshen91 added 9 commits February 10, 2022 07:55

initial commit toolbar button component

66efef9

add story and mdx

6ab9f6a

update exports"

eef1020

fix import

fde66b9

linting fix

4530e41

fix export consts in index files

9331527

migrate solutions toolbar

7e722b5

fix namespace issue

b38e98c

add mdx file

f6154e6

rshen91 force-pushed the migrate-toolbar-button branch from e6742ef to f6154e6 Compare February 10, 2022 15:01

kibanamachine and others added 4 commits February 10, 2022 10:02

Merge branch 'main' into migrate-toolbar-button

54612d7

fix docs source for story to mdx

345f987

Merge remote-tracking branch 'origin/migrate-toolbar-button' into mig…

d1d193d

…rate-toolbar-button

fix incorrect import

b288877

majagrubic reviewed Feb 11, 2022

View reviewed changes

rshen91 added 3 commits February 14, 2022 07:47

Merge remote-tracking branch 'upstream/main' into migrate-toolbar-button

264b5e4

fix storybook docs

2304fb9

add id to props

17c81e7

clintandrewhall reviewed Feb 14, 2022

View reviewed changes

partial code review changes

75c9821

kibanamachine and others added 5 commits February 15, 2022 08:59

Merge branch 'main' into migrate-toolbar-button

56bcc47

remove scss

e27b041

Merge remote-tracking branch 'origin/migrate-toolbar-button' into mig…

be3306e

…rate-toolbar-button

removing sass and refactoring strings component

7fa17de

change namespace to sharedUX

1c8b762

rshen91 requested review from clintandrewhall and majagrubic February 15, 2022 19:29

rshen91 marked this pull request as ready for review February 15, 2022 19:29

rshen91 added v8.2.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 15, 2022

cchaos reviewed Feb 15, 2022

View reviewed changes

majagrubic approved these changes Feb 16, 2022

View reviewed changes

add icon and text to story toolbar button

f89d158

clintandrewhall requested changes Feb 16, 2022

View reviewed changes

rshen91 added 2 commits February 16, 2022 12:24

remove unused declarations

eabf375

Merge branch 'main' into migrate-toolbar-button

38b30d4

rshen91 requested a review from cchaos February 16, 2022 19:26

rshen91 closed this Feb 17, 2022

rshen91 mentioned this pull request Feb 17, 2022

[shared-ux] Migrate solution toolbar button #125998

Merged

7 tasks

rshen91 deleted the migrate-toolbar-button branch March 2, 2022 15:01


		import { i18n } from '@kbn/i18n';

		export const ComponentStrings = {

[shared-ux] Migrate toolbar button component #124372

[shared-ux] Migrate toolbar button component #124372

Conversation

rshen91 commented Feb 2, 2022 • edited Loading

Summary

Checklist

For maintainers

rshen91 commented Feb 3, 2022

rshen91 commented Feb 7, 2022

rshen91 commented Feb 8, 2022

majagrubic Feb 11, 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

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

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

rshen91 commented Feb 15, 2022

cchaos left a comment

Choose a reason for hiding this comment

rshen91 commented Feb 15, 2022

majagrubic left a comment

Choose a reason for hiding this comment

clintandrewhall commented Feb 16, 2022

clintandrewhall left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 16, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

History

rshen91 commented Feb 17, 2022

rshen91 commented Feb 2, 2022 •

edited

Loading

majagrubic Feb 11, 2022 •

edited

Loading