[Shared-UX] Migrate Toolbar component from presentation to shared-ux #137224

rshen91 · 2022-07-26T18:32:47Z

Summary

The Toolbar component was previously named SolutionToolbar component. It is currently only used in x-pack/plugins/canvas/public/components/workpad_header/workpad_header.component.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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

For maintainers

This was checked for breaking API changes and was labeled appropriately

…-fix'

…ckage

…-fix'

…ckage

…-fix'

clintandrewhall

An awesome addition! Just a few changes and thoughts.

clintandrewhall · 2022-07-29T13:22:04Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+  );
+
+  return (
+    <EuiFlexGroup id={`kbnPresentationToolbar__solutionToolbar`} gutterSize="s">


This ID is probably not necessary.

clintandrewhall · 2022-07-29T13:22:44Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+  extraButtons?: Array<ReactElement<typeof PrimaryButton | typeof ToolbarPopover> | undefined>;
+}
+
+export interface Props {


clintandrewhall · 2022-07-29T13:22:50Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+  children: NamedSlots;
+}
+
+export const Toolbar = ({ children }: Props) => {


clintandrewhall · 2022-07-29T13:37:34Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+import { IconButtonGroup, PrimaryButton } from '../buttons';
+import { ToolbarPopover } from '../popover';
+
+interface NamedSlots {


This ends up getting exported as part of the Props interface, so it probably needs docs and inline docs per parameter:

/** Some description **/ primaryButton: ...

clintandrewhall · 2022-07-29T13:39:28Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+import { ToolbarPopover } from '../popover';
+
+interface NamedSlots {
+  primaryButton: ReactElement<typeof PrimaryButton | typeof ToolbarPopover>;


nit: we repeat typeof PrimaryButton | typeof ToolbarPopover in a number of places... let's introduce a type:

type Button = typeof PrimaryButton | typeof ToolbarPopover`

clintandrewhall · 2022-07-29T13:41:29Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+export const Toolbar = ({ children }: Props) => {
+  const { primaryButton, iconButtonGroup, extraButtons = [] } = children;
+
+  const extra = extraButtons.map((button, index) =>


nit: we don't currently restrict this array to any particular length, but it stands to reason there's a maximum. We might want to consider logging a warning-- or throwing an error-- if the number of extra buttons exceeds a certain design limit. Thoughts @cchaos ?

I added an error in fa629f2 for over 120 buttons. I'm open to lowering that number to...50? Would logging a warning be better? 😅

I would log a warning, yes... and I was thinking more like 5, maybe as many as 7. Maybe @ryankeairns can weigh in, but we don't need to block this PR from going in.

…-ref HEAD~1..HEAD --fix'

…ckage

packages/shared-ux/button_toolbar/README.mdx

majagrubic · 2022-08-25T13:26:23Z

packages/shared-ux/button_toolbar/src/index.ts

+
+export { Toolbar } from './toolbar';
+export type { ToolbarProps } from './toolbar';
+export type { Button } from './toolbar';


I think this would better be called ToolbarButton or something.

I can get behind that thanks! 3247796

majagrubic · 2022-08-25T13:29:01Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.test.tsx

+
+describe('<Toolbar />', () => {
+  test('is rendered', () => {
+    const primaryButton = <PrimaryButton label="Create chart" onClick={() => 'click'} />;


nit: onClick be just empty function if we're not testing if it can be clicked. but can we add a test that click actually calls the function?

Good call - 379590c I got a little goofed up trying to get only the button clicked and I think this test nails it thanks!

majagrubic · 2022-08-25T13:32:36Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+
+  if (extraButtons.length > 120) {
+    throw new Error(
+      'There are over 120 extra buttons. Please consider limiting the number of buttons.'


Should the error be localized?

Probably, yes! Thank you! 3247796 I think I did it right? 🤞🏻

…-ref HEAD~1..HEAD --fix'

…ckage

…-fix'

rshen91 · 2022-08-29T13:16:52Z

@elasticmachine merge upstream

majagrubic

I don't have any additional comments, so on my end this is approved. Tested this by running storybook in Chrome on Mac OSX.

clintandrewhall

Great work!

clintandrewhall · 2022-08-29T19:45:18Z

packages/shared-ux/button_toolbar/src/toolbar/toolbar.tsx

+export const Toolbar = ({ children }: Props) => {
+  const { primaryButton, iconButtonGroup, extraButtons = [] } = children;
+
+  const extra = extraButtons.map((button, index) =>


I would log a warning, yes... and I was thinking more like 5, maybe as many as 7. Maybe @ryankeairns can weigh in, but we don't need to block this PR from going in.

kibana-ci · 2022-08-29T20:22:39Z

💚 Build Succeeded

Metrics [docs]

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-button-toolbar`	6	8	+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/shared-ux-button-toolbar`	20	25	+5

History

💚 Build #67992 succeeded 3eda666
💚 Build #67495 succeeded e56c646
💛 Build #67073 was flaky b753e86
💚 Build #66855 succeeded df3ac81
💚 Build #66433 succeeded 584cf07
💚 Build #66094 succeeded 316bdcb

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

cc @rshen91

kibanamachine · 2022-08-29T20:26:11Z

💔 All backports failed

Status	Branch	Result
❌	8.4	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 137224

Questions ?

Please refer to the Backport tool documentation

…lastic#137224)

rshen91 and others added 5 commits July 26, 2022 12:31

initial commit

d6258d7

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

58f006c

…-fix'

update story

602ae1e

Merge remote-tracking branch 'origin/toolbar-package' into toolbar-pa…

81a4079

…ckage

Merge remote-tracking branch 'upstream/main' into toolbar-package

a74445d

rshen91 self-assigned this Jul 27, 2022

kibanamachine and others added 10 commits July 27, 2022 10:31

Merge branch 'main' into toolbar-package

fa02dcd

lint fix

a1e042c

Merge remote-tracking branch 'origin/toolbar-package' into toolbar-pa…

64d4d37

…ckage

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

54bbf14

…-fix'

update test

850dd16

Merge remote-tracking branch 'origin/toolbar-package' into toolbar-pa…

dd3f12e

…ckage

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

f02dc3f

…-fix'

Merge branch 'main' into toolbar-package

df574e0

Merge branch 'main' into toolbar-package

93a01e4

first pass at mdx file

fb9e938

clintandrewhall requested changes Jul 29, 2022

View reviewed changes

rshen91 and others added 5 commits July 29, 2022 10:28

Merge remote-tracking branch 'upstream/main' into toolbar-package

c3850bd

add comments

715e4fa

Merge remote-tracking branch 'upstream/main' into toolbar-package

a8f40ec

add Button type

2c4c902

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

324b26d

…-ref HEAD~1..HEAD --fix'

rshen91 mentioned this pull request Aug 1, 2022

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

Open

15 tasks

rshen91 added 6 commits August 1, 2022 08:27

fix

12d653d

Merge remote-tracking branch 'origin/toolbar-package' into toolbar-pa…

1f1a7c0

…ckage

Merge branch 'main' into toolbar-package

4d1176d

missed test update

dcd451e

add error for over 120 buttons

fa629f2

Merge branch 'main' into toolbar-package

093d9a5

rshen91 requested a review from clintandrewhall August 2, 2022 21:32

rshen91 added v8.4.0 v8.5.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 16, 2022

rshen91 added 4 commits August 18, 2022 10:39

Merge branch 'main' into toolbar-package

316bdcb

Merge remote-tracking branch 'upstream/main' into toolbar-package

584cf07

Merge remote-tracking branch 'upstream/main' into toolbar-package

df3ac81

Merge remote-tracking branch 'upstream/main' into toolbar-package

b753e86

majagrubic reviewed Aug 25, 2022

View reviewed changes

packages/shared-ux/button_toolbar/README.mdx Outdated Show resolved Hide resolved

majagrubic reviewed Aug 25, 2022

View reviewed changes

rshen91 and others added 6 commits August 25, 2022 08:43

Merge remote-tracking branch 'upstream/main' into toolbar-package

92a7e35

code review

3247796

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

80931f6

…-ref HEAD~1..HEAD --fix'

add test for clicking the button within the toolbar click event

379590c

Merge remote-tracking branch 'origin/toolbar-package' into toolbar-pa…

55bd1ad

…ckage

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

e56c646

…-fix'

rshen91 requested a review from majagrubic August 25, 2022 19:31

Merge branch 'main' into toolbar-package

3eda666

majagrubic approved these changes Aug 29, 2022

View reviewed changes

rshen91 enabled auto-merge (squash) August 29, 2022 19:18

Merge branch 'main' into toolbar-package

1612efa

clintandrewhall approved these changes Aug 29, 2022

View reviewed changes

rshen91 merged commit 1ebfd79 into elastic:main Aug 29, 2022

rshen91 deleted the toolbar-package branch August 29, 2022 20:25

rshen91 added the backport:skip This commit does not require backporting label Aug 29, 2022

Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022

[Shared-UX] Migrate Toolbar component from presentation to shared-ux (e…

f3a2668

…lastic#137224)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Shared-UX] Migrate Toolbar component from presentation to shared-ux #137224

[Shared-UX] Migrate Toolbar component from presentation to shared-ux #137224

rshen91 commented Jul 26, 2022 •

edited by kibanamachine

Loading

clintandrewhall left a comment

clintandrewhall Jul 29, 2022

clintandrewhall Jul 29, 2022

clintandrewhall Jul 29, 2022

clintandrewhall Jul 29, 2022

clintandrewhall Jul 29, 2022

clintandrewhall Jul 29, 2022

rshen91 Aug 1, 2022

clintandrewhall Aug 29, 2022

majagrubic Aug 25, 2022

rshen91 Aug 25, 2022

majagrubic Aug 25, 2022

rshen91 Aug 25, 2022

majagrubic Aug 25, 2022

rshen91 Aug 25, 2022

rshen91 commented Aug 29, 2022

majagrubic left a comment •

edited

Loading

clintandrewhall left a comment

clintandrewhall Aug 29, 2022

kibana-ci commented Aug 29, 2022

API count

kibanamachine commented Aug 29, 2022

[Shared-UX] Migrate Toolbar component from presentation to shared-ux #137224

[Shared-UX] Migrate Toolbar component from presentation to shared-ux #137224

Conversation

rshen91 commented Jul 26, 2022 • edited by kibanamachine Loading

Summary

Checklist

For maintainers

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 Aug 29, 2022

majagrubic left a comment • edited Loading

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

kibana-ci commented Aug 29, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

kibanamachine commented Aug 29, 2022

💔 All backports failed

Manual backport

Questions ?

rshen91 commented Jul 26, 2022 •

edited by kibanamachine

Loading

majagrubic left a comment •

edited

Loading