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.

Sign up for GitHub

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

Jump to bottom

[shared-ux][packages] 2. Create Storybook Package #127548

Merged
merged 3 commits into from
Mar 14, 2022
Merged

[shared-ux][packages] 2. Create Storybook Package #127548

merged 3 commits into from
Mar 14, 2022

Conversation

clintandrewhall
Copy link
Contributor

Summary

This is a subset of the work in #127419 for ease of review and commit history.
Depends on #127546

This PR refactors the Shared UX Storybook configuration to use @kbn/shared-ux-services, and moves it from the shared_ux plugin to a package.

Why?

With only a few components, we've run into issues with circular dependencies. Our plugin is consuming code and types from other plugins, and once we do, those plugins can't use our plugin or its components. To mitigate this, we need to keep Shared UX as stateless and dependency free as possible.

Talking with ops, it became clear our assets don't actually need a plugin lifecycle to operate. This PR is one step in decoupling our components, services and utilities to packages.

TODO

  • Complete .mdx documentation.

@clintandrewhall clintandrewhall added review loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:critical This issue should be addressed immediately due to a critical level of impact on the product. backport:skip This commit does not require backporting v8.2.0 Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Mar 11, 2022
@clintandrewhall clintandrewhall requested a review from a team March 11, 2022 21:29
@clintandrewhall clintandrewhall requested a review from a team as a code owner March 11, 2022 21:29
@clintandrewhall clintandrewhall mentioned this pull request Mar 11, 2022
Closed
1 task
@clintandrewhall clintandrewhall force-pushed the shared_ux/packages/storybook branch from 9899ec7 to ad6a03a Compare March 11, 2022 21:41
@clintandrewhall clintandrewhall mentioned this pull request Mar 11, 2022
Merged
1 task
@clintandrewhall clintandrewhall force-pushed the shared_ux/packages/services branch from d27684c to 13efeb8 Compare March 11, 2022 21:51
@clintandrewhall clintandrewhall force-pushed the shared_ux/packages/storybook branch from ad6a03a to f9571c7 Compare March 11, 2022 21:51
This was referenced Mar 11, 2022
Merged
Closed
majagrubic
majagrubic reviewed Mar 14, 2022
View reviewed changes
packages/kbn-shared-ux-storybook/src/config/main.ts
...defaultConfig,
stories: [
'../../../kbn-shared-ux*/**/*.stories.+(tsx|mdx)',
'../../../../src/plugins/shared_ux/**/*.stories.+(tsx|mdx)',
Copy link
Contributor

Choose a reason for hiding this comment

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

It is ok for a package to have a reference to plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now we're not really counting this as a "reference". Storybook is very development-time specific, nothing about it ends up in the build, and this doesn't exactly load code or integrate with the build systems, node module loaders, etc...

We're okay with storybook packages reading stories from throughout the repo. It seems to make sense considering the tool, so Operations has "blessed" this exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add this to the documentation for this package, to clear up any confusion.

majagrubic
majagrubic reviewed Mar 14, 2022
View reviewed changes
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.

I don't understand why we need this package. To provide implementation for the services for use in storybook files? Why can't this be a part of services pacakge. Or at least, should the package be called storybook-services?

@clintandrewhall
Copy link
Contributor Author

@majagrubic The reason for this package is because, when you depend on a package, you get all of the code that comes with it... just like an npm package. So splitting this code out specifically for Storybook use cases will ensure that consumers of other packages don't depend on this code, as well.

Packages can be further and further divided by subject matter or dependency. For example, I expect @kbn/shared-ux-components to be further divided in the near future as the package grows, (e.g. @kbn/shared-ux-empty-state and @kbn/shared-ux-buttons.

@clintandrewhall @kibanamachine
[shared-ux][packages] 3. Create Utility Package (#127549)
fa847de 
* [shared-ux][packages] 3. Create Utility Package

* [shared-ux][packages] 4. Create Components Package (#127551)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@clintandrewhall clintandrewhall requested a review from a team as a code owner March 14, 2022 21:18
@clintandrewhall clintandrewhall merged commit 2976de7 into shared_ux/packages/services Mar 14, 2022
@clintandrewhall clintandrewhall deleted the shared_ux/packages/storybook branch March 14, 2022 21:25
@kibana-ci
Copy link
Collaborator

kibana-ci commented Mar 14, 2022 •
edited
Loading

⏳ Build in-progress, with failures

Failed CI Steps

Metrics [docs]

‼️ ERROR: metrics for 6e7d84e were not reported

History

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

clintandrewhall added a commit that referenced this pull request Mar 16, 2022
@clintandrewhall @kibanamachine
[sharedUX] Move to Package-based Architecture (#127546)
71af73c 
* [shared-ux][packages] 1. Create Services Package

* Address review feedback

* [shared-ux][packages] 2. Create Storybook Package (#127548)

* [shared-ux][packages] 2. Create Storybook Package

* [shared-ux][packages] 3. Create Utility Package (#127549)

* [shared-ux][packages] 3. Create Utility Package

* [shared-ux][packages] 4. Create Components Package (#127551)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Merging

* Adding docs

* A few fixes

* Fix TS types

* Fix TS types

* Fix i18n

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
@clintandrewhall @kibanamachine @maksimkovalev
[sharedUX] Move to Package-based Architecture (elastic#127546)
e2d45ae 
* [shared-ux][packages] 1. Create Services Package

* Address review feedback

* [shared-ux][packages] 2. Create Storybook Package (elastic#127548)

* [shared-ux][packages] 2. Create Storybook Package

* [shared-ux][packages] 3. Create Utility Package (elastic#127549)

* [shared-ux][packages] 3. Create Utility Package

* [shared-ux][packages] 4. Create Components Package (elastic#127551)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Merging

* Adding docs

* A few fixes

* Fix TS types

* Fix TS types

* Fix i18n

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spalger spalger spalger left review comments

@majagrubic majagrubic majagrubic approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium 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.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clintandrewhall @kibana-ci @spalger @majagrubic