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

Move @automattic/components' Storybook to the root Storybook #76007

Merged
merged 13 commits into from
Apr 21, 2023
Merged

Move @automattic/components' Storybook to the root Storybook #76007

merged 13 commits into from
Apr 21, 2023

Conversation

okmttdhr
Copy link
Member

@okmttdhr okmttdhr commented Apr 20, 2023 •
edited
Loading

Related to #75167, #75943

Proposed Changes

Testing Instructions

  • Run yarn storybook:start locally
  • Access to the stories under @automattic/components directory
Screen.Recording.2023-04-20.at.16.13.57.mov

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@github-actions
Copy link

github-actions bot commented Apr 20, 2023 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-64369
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-64369&env=jetpack

@okmttdhr okmttdhr mentioned this pull request Apr 20, 2023
Merged
7 tasks
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@okmttdhr okmttdhr marked this pull request as ready for review April 20, 2023 07:20
@okmttdhr okmttdhr requested a review from a team April 20, 2023 07:21
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 20, 2023
@okmttdhr okmttdhr requested a review from a team April 20, 2023 07:21
@okmttdhr okmttdhr self-assigned this Apr 20, 2023
tyxla
tyxla approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is looking great 👍

Just left a documentation suggestion.

Also, did you intend to merge into the other PR, or is the idea to merge it first, and then rebase this onto trunk?

In any case, this works well in my testing and code looks great 🚀

packages/components/package.json
@@ -67,7 +66,6 @@
"scripts": {
"clean": "tsc --build ./tsconfig.json ./tsconfig-cjs.json --clean && rm -rf dist",
"build": "tsc --build ./tsconfig.json ./tsconfig-cjs.json && copy-assets",
"prepack": "yarn run clean && yarn run build",
"storybook": "start-storybook"
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep in mind the READMEs and documentation that we need to update:

https://github.com/Automattic/wp-calypso/blob/trunk/packages/components/README.md#L44

Ideally, we should add a more central section that leads to the central Storybook instance, somewhere in /docs for example in components.md.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for pointing this out, I updated the code about the existing command in this PR. 722e121

And I agree that we must update the document, such as https://github.com/Automattic/wp-calypso/blob/trunk/docs/components.md and https://github.com/Automattic/wp-calypso/blob/trunk/docs/component-libraries.md.

noahtallen
noahtallen reviewed Apr 20, 2023
View reviewed changes
packages/components/src/chart-uplot/stories/index.stories.tsx Outdated
@@ -3,7 +3,7 @@
import { useRef } from 'react';
import UplotChart from '../';

export default { title: 'Chart (uPlot)' };
export default { title: 'packages/@automattic/components/Chart (uPlot)' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor feedback since naming can be easily adjusted whenever: I think packages and @automattic mean about the same thing here. packages/ is the directory in wp-calypso where packages are developed, and @automattic/ is the namespace we use for publishing them to npm. I don't think we need both.

I think I'd prefer sticking with packages and remove @automattic. (this is subjective feedback though, so it's non-blocking!)

noahtallen
noahtallen approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@noahtallen noahtallen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice. Very easy!

I would recommend updating this as well to mention the new command:

wp-calypso/package.json

Line 90 in 500b5d1

"components:storybook:start": "echo 'Deprecated, run `yarn workspace @automattic/components run storybook` instead'",

@okmttdhr okmttdhr force-pushed the update/storybook-sample-another-package branch from 31f8bb6 to 1556b50 Compare April 21, 2023 04:54
Base automatically changed from update/storybook-sample-another-package to trunk April 21, 2023 05:10
@okmttdhr okmttdhr force-pushed the update/storybook-a8c-components branch from 586e24c to 722e121 Compare April 21, 2023 05:49
@okmttdhr okmttdhr merged commit ed6d404 into trunk Apr 21, 2023
@okmttdhr okmttdhr deleted the update/storybook-a8c-components branch April 21, 2023 06:17
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 21, 2023
@jsnmoon
Copy link
Contributor

jsnmoon commented May 9, 2023

Hey folks, this PR missed some components from the components package:

  • packages/components/src/app-promo-card/stories/index.stories.jsx
  • packages/components/src/component-swapper/stories/index.stories.jsx
  • packages/components/src/highlight-cards/stories/card.stories.jsx
  • packages/components/src/highlight-cards/stories/cards.stories.jsx
  • packages/components/src/horizontal-bar-list/stories/index.stories.jsx
  • packages/components/src/icons/stories/index.stories.jsx
  • packages/components/src/number-formatters/stories/index.stories.jsx
  • packages/components/src/post-stats-card/stories/index.stories.jsx

It looks like only stories written in TypeScript were ported over. Was this intentional?

@okmttdhr
Copy link
Member Author

Hello @jsnmoon !
Not on purpose, but I forgot to check them 😇
Those files should be included in .storybook/main.js or converted into TypeScript.

@jsnmoon jsnmoon mentioned this pull request May 10, 2023
Merged
7 tasks
@mirka mirka mentioned this pull request Jan 6, 2025
Merged
8 tasks
@mirka mirka mentioned this pull request Jan 13, 2025
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahtallen noahtallen noahtallen approved these changes

@tyxla tyxla tyxla approved these changes

Assignees

@okmttdhr okmttdhr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@okmttdhr @matticbot @jsnmoon @noahtallen @tyxla