Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Shared UX] Move No Data Cards to packages #134503

Merged
merged 9 commits into from
Jun 21, 2022

Conversation

clintandrewhall
Copy link
Contributor

Summary

This PR:

  • moves the NoDataCard and NoDataElasticAgentCard to their own packages.
  • sets up and integrates Storybook and Jest mocks for NoDataElasticAgentCard.
  • makes relevant changes to dependent components.

Jun-15-2022 12-28-28

@clintandrewhall clintandrewhall added review loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.4.0 labels Jun 15, 2022
@clintandrewhall clintandrewhall requested a review from a team June 15, 2022 17:30
@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch from d623810 to bc5ff11 Compare June 15, 2022 17:55
@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch from bc5ff11 to 1849bf9 Compare June 16, 2022 01:41
@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch 3 times, most recently from 143316b to 1886fcd Compare June 16, 2022 03:53
@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch from 1886fcd to a770167 Compare June 16, 2022 04:14
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested this in storybook in Chrome on Mac OSX, works as expected.
Made a quick pass through the code, but not too detailed, since it's just moving files around I assume there were no major changes. Looks like Solutions are using the card already, so we'll need to make changes there as well.

@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch from b7e2916 to ed5dd0f Compare June 16, 2022 15:52
@clintandrewhall
Copy link
Contributor Author

It took more work to get this over the line than I would have liked... in commits since @majagrubic approved this PR, I:

  • I adjusted how and where contexts are combined. I realize these contexts are only going to be in this configuration for a short period, but I still would like a sanity check.
  • you can't use shallow with useContext, so the snapshot for NoDataPage was enormous, and I had to remove it. issue
  • I fixed a React props bug with RedirectAppLinks that was causing props to be applied (however benignly) to a div element.
  • I added React strict mode to our Storybooks to ensure we don't break React norms.

So I'm requesting @elastic/shared-ux have another look at this before I commit it. Thanks!

@@ -27,4 +22,6 @@ type Props = Omit<ComponentProps, 'navigateToUrl' | 'currentAppId'>;
* </RedirectAppLinks>
* ```
*/
export const RedirectAppLinks = (props: Props) => <Component {...useServices()} {...props} />;
export const RedirectAppLinks: FC<{}> = ({ children }) => (
<Component {...useServices()}>{children}</Component>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Code looks good, haven't tested it this time.

@clintandrewhall clintandrewhall force-pushed the shared_ux/no_data_card branch from c7b2e55 to d55e52d Compare June 21, 2022 14:55
@clintandrewhall clintandrewhall enabled auto-merge (squash) June 21, 2022 15:57
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #34 / machine learning - data frame analytics regression saved search creation with kuery query runs the analytics job and displays it correctly in the job list

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 419 421 +2
dataViewManagement 149 182 +33
discover 588 590 +2
home 141 175 +34
kibanaOverview 173 175 +2
lens 920 922 +2
observability 491 525 +34
securitySolution 3072 3104 +32
visualizations 371 373 +2
total +143

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-card-no-data - 7 +7

Async chunks

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

id before after diff
dashboard 391.4KB 391.6KB +194.0B
dataViewManagement 123.1KB 160.6KB +37.5KB
discover 487.8KB 488.0KB +194.0B
home 102.1KB 139.2KB +37.2KB
kibanaOverview 95.7KB 95.9KB +195.0B
lens 1.2MB 1.2MB +194.0B
observability 522.7KB 559.9KB +37.3KB
securitySolution 5.3MB 5.3MB +37.7KB
visualizations 252.9KB 253.9KB +987.0B
total +151.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/shared-ux-card-no-data - 1 +1
@kbn/shared-ux-components 2 1 -1
kibana 371 374 +3
total +3

Page load bundle

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

id before after diff
home 10.4KB 10.4KB +1.0B
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-card-no-data - 14 +14

History

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

@clintandrewhall clintandrewhall merged commit e1eb3db into elastic:main Jun 21, 2022
@clintandrewhall clintandrewhall self-assigned this Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants