[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025

majagrubic · 2022-03-07T13:55:49Z

Summary

This is a first PR in migrating PageTemplate from kibana_react to shared_ux plugin. This PR tackles NoDataCard component. It is not exported from the plugin, as it's not used outside of PageTemplate.

Part of #122961

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

Tested locally - great storybook addition! LGTM

cchaos

Just a couple quick comments, but in general should there also be Markdown docs pages being created next to these components?

cchaos · 2022-03-07T17:31:36Z

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/types.ts

+import { EuiCardProps } from '@elastic/eui';
+import { MouseEventHandler, ReactNode } from 'react';
+
+export type NoDataPageActions = Partial<EuiCardProps> & {


Why separate the component type into a separate file?

Also since it's the direct props of NoDataCard, let's name it consistently:

Suggested change

export type NoDataPageActions = Partial<EuiCardProps> & {

export type NoDataCardProps = Partial<EuiCardProps> & {

Because this type will be reused for ElasticAgentCard once I migrate it over, so that component can read from types file as well.

...plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.tsx

...shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.stories.tsx

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/index.ts

...shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.stories.tsx

...plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.tsx

clintandrewhall · 2022-03-07T17:23:50Z

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/types.ts

+  /**
+   * Applies the `Recommended` beta badge and makes the button `fill`
+   */
+  recommended?: boolean;


Suggested change

recommended?: boolean;

isRecommended?: boolean;

I don't want to be changing prop names at this time, as I don't know where down the line in PageTemplate is this component used, and renaming them all will add additional work.

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/types.ts

clintandrewhall · 2022-03-07T17:32:31Z

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/types.ts

+  /**
+   * Provide just a string for the button's label, or a whole component
+   */
+  button?: string | ReactNode;


@cchaos How does EUI handle this kind of flexibility? Is there a pattern?

@majagrubic Be sure this is the TS type you want to use.

https://dev.to/fromaline/jsxelement-vs-reactelement-vs-reactnode-2mh2
https://github.com/typescript-cheatsheets/react#useful-react-prop-type-examples

I'm not sure there's a specific pattern, but the idea here is that the consumer can supply just the button label (string) or replace the whole button their own (in case they don't want to use the same medium, filled button). I guess technically string is included in ReactNode, so maybe that's duplicative. But I think the explicitness here is helpful because an element would replace the whole default EuiButton where the string is just the label. Up to you all if you think there's a better pattern.

I guess I was thinking maybe string | EuiButtonProps.

<Card button="Click Me" /> <Card button={{ label: 'Click Me', onClick: () => {}, fill: true }} />

Then we'd have some control over precisely what component we rendered there, and could force a fill or color to keep the usage consistent, now or in the future.

It's probably minor, but I'm one who leans toward keeping the contract strict at the start, and then expanding outward as necessary, to avoid large-scale refactors later.

You could do that. But specifically EuiButton doesn't have a label prop, it's label is just the children, so you'd likely need to append this prop type or rely on consumers to pass children as a key.

Oh yeah, I think the problem with that, that I'm now remembering, is that you can't go from say an EuiButton to EuiEmptyButton by props alone since they're completely different components. So an example is if the footer button is actually not a CTA we want to promote, or say we need to provide 2 actions instead of just one, there's no way to remove the default button without replacing it completely with a ReactNode.

src/plugins/shared_ux/public/components/page_template/no_data_page/no_data_card/types.ts

majagrubic · 2022-03-08T10:09:54Z

@clintandrewhall do you know how and why app-services got automatically tagged for a review of this?

majagrubic · 2022-03-08T14:40:02Z

@elasticmachine merge upstream

cchaos

Thanks @majagrubic !

...shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.stories.tsx

…page/no_data_card/no_data_card.stories.tsx Co-authored-by: Caroline Horn <[email protected]>

majagrubic · 2022-03-09T17:49:17Z

@elasticmachine merge upstream

kibana-ci · 2022-03-09T19:12:59Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #28858 failed 6e3679b
💚 Build #28497 succeeded 346ec1f
💚 Build #28405 succeeded 71b8fc7
💔 Build #28334 failed 112e6d9
💔 Build #28303 failed c55c35b616cde2c1a447626c38811f3aa59386c8

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

kibanamachine · 2022-03-11T19:56:18Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127025 or prevent reminders by adding the backport:skip label.

Maja Grubic added 4 commits March 7, 2022 14:55

[SharedUX] Migrate PageTemplate > NoData > NoDataCard

94a2b3e

Add deprecation warning

1e197b9

Fix directory structure

f3eae30

Fix i18n

bec6e63

majagrubic changed the title ~~[SharedUX] Migrate PageTemplate > NoData > NoDataCard~~ [SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard Mar 7, 2022

majagrubic added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Mar 7, 2022

majagrubic marked this pull request as ready for review March 7, 2022 16:47

majagrubic requested a review from a team March 7, 2022 16:47

majagrubic requested a review from a team as a code owner March 7, 2022 16:47

majagrubic requested a review from rshen91 March 7, 2022 16:47

rshen91 approved these changes Mar 7, 2022

View reviewed changes

cchaos reviewed Mar 7, 2022

View reviewed changes

Applying Caroline's suggestions

24bcf27

clintandrewhall reviewed Mar 7, 2022

View reviewed changes

majagrubic force-pushed the shared-ux/no-data-card branch from c55c35b to ba87e76 Compare March 8, 2022 11:07

Fix types

112e6d9

majagrubic force-pushed the shared-ux/no-data-card branch from ba87e76 to 112e6d9 Compare March 8, 2022 11:08

Merge branch 'main' into shared-ux/no-data-card

71b8fc7

majagrubic requested a review from clintandrewhall March 8, 2022 17:07

Fix typescripting

346ec1f

majagrubic requested a review from cchaos March 9, 2022 06:34

cchaos approved these changes Mar 9, 2022

View reviewed changes

...shared_ux/public/components/page_template/no_data_page/no_data_card/no_data_card.stories.tsx Outdated Show resolved Hide resolved

Update src/plugins/shared_ux/public/components/page_template/no_data_…

6e3679b

…page/no_data_card/no_data_card.stories.tsx Co-authored-by: Caroline Horn <[email protected]>

Merge branch 'main' into shared-ux/no-data-card

1ef414c

majagrubic merged commit 8782ced into elastic:main Mar 9, 2022

majagrubic deleted the shared-ux/no-data-card branch March 9, 2022 19:46

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 11, 2022

clintandrewhall added the backport:skip This commit does not require backporting label Mar 11, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025

[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025

majagrubic commented Mar 7, 2022 •

edited

Loading

rshen91 left a comment

cchaos left a comment

cchaos Mar 7, 2022

majagrubic Mar 7, 2022 •

edited

Loading

clintandrewhall Mar 7, 2022

majagrubic Mar 8, 2022

clintandrewhall Mar 7, 2022

cchaos Mar 7, 2022

clintandrewhall Mar 7, 2022 •

edited

Loading

cchaos Mar 7, 2022

cchaos Mar 7, 2022

majagrubic commented Mar 8, 2022

majagrubic commented Mar 8, 2022

cchaos left a comment

majagrubic commented Mar 9, 2022

kibana-ci commented Mar 9, 2022

kibanamachine commented Mar 11, 2022

	export type NoDataPageActions = Partial<EuiCardProps> & {
	export type NoDataCardProps = Partial<EuiCardProps> & {

[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025

[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025

Conversation

majagrubic commented Mar 7, 2022 • edited Loading

Summary

Checklist

For maintainers

rshen91 left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

cchaos Mar 7, 2022

Choose a reason for hiding this comment

majagrubic Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

clintandrewhall Mar 7, 2022

Choose a reason for hiding this comment

majagrubic Mar 8, 2022

Choose a reason for hiding this comment

clintandrewhall Mar 7, 2022

Choose a reason for hiding this comment

cchaos Mar 7, 2022

Choose a reason for hiding this comment

clintandrewhall Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

cchaos Mar 7, 2022

Choose a reason for hiding this comment

cchaos Mar 7, 2022

Choose a reason for hiding this comment

majagrubic commented Mar 8, 2022

majagrubic commented Mar 8, 2022

cchaos left a comment

Choose a reason for hiding this comment

majagrubic commented Mar 9, 2022

kibana-ci commented Mar 9, 2022

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Mar 11, 2022

majagrubic commented Mar 7, 2022 •

edited

Loading

majagrubic Mar 7, 2022 •

edited

Loading

clintandrewhall Mar 7, 2022 •

edited

Loading