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

Framework: Add Component Storybook (tweaks) #17762

Merged
merged 2 commits into from
Oct 8, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Oct 4, 2019

Description

Some tweaks for the general setup of #17475.

Changes included:

  • Eslint rule updated consolidated with the existing one
  • scripts for building and watching packages updated to ignore Storybook files
  • @storybook/addon-docs downgraded to the same version as other packages
  • babel-loader removed from devDependencies - it isn't explicitly used so dependencies should have more freedom to pick a proper version
  • design-system:build runs also npm run build:packages as one of the steps to align with the playground and to fix the issue which will pop up as soon as we start using internal dependencies in stories
  • @wordpress/babel-preset-default preset added to the custom Babel config so we don't have to explicitly load React as a dependnecy but rather use @wordpress/element

Unrelated cleanup changes:

  • Lerna config updated to ignore some missed files

How has this been tested?

The new storybook was verified to work in development (npm run design-system:dev) and production (npm run design-system:build) modes.

@gziolo gziolo mentioned this pull request Oct 4, 2019
5 tasks
@gziolo gziolo changed the base branch from master to add/storybook-for-components October 4, 2019 06:41
@gziolo gziolo changed the base branch from add/storybook-for-components to master October 4, 2019 06:42
@gziolo gziolo force-pushed the add/storybook-for-components-2 branch from e298c15 to 13c79dc Compare October 4, 2019 06:46
@gziolo gziolo changed the base branch from master to add/storybook-for-components October 4, 2019 06:49
@gziolo gziolo changed the title Framework: Add Component Storybook (test) Framework: Add Component Storybook (tweaks) Oct 4, 2019
@gziolo gziolo added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Oct 4, 2019
@gziolo gziolo force-pushed the add/storybook-for-components branch from 53b1101 to d491df8 Compare October 4, 2019 10:04
@gziolo
Copy link
Member Author

gziolo commented Oct 4, 2019

I see a failing unit test:

https://travis-ci.com/WordPress/gutenberg/jobs/241941170#L5643

FAIL packages/env/tests/cli.test.js
  ● env cli › handles successful commands with messages.
    expect(jest.fn()).toHaveBeenCalledWith(expected)
    Expected mock function to have been called with:
      "success message (in 0s 0ms)"
    as argument 1, but it was called with
      "success message (in 0s 1ms)".
      72 | 		const { spinner } = env.start.mock.calls[ 0 ][ 0 ];
      73 | 		await env.start.mock.results[ 0 ].value;
    > 74 | 		expect( spinner.succeed ).toHaveBeenCalledWith( 'success message (in 0s 0ms)' );
         | 		                          ^
      75 | 	} );
      76 | 	it( 'handles successful commands with spinner text.', async () => {
      77 | 		env.start.mockResolvedValueOnce();
      at Object.toHaveBeenCalledWith (packages/env/tests/cli.test.js:74:29)

It looks like this test depends on the execution time so it might fail from time to time. We probably should limit this check to whether this string starts with success message.

@gziolo gziolo force-pushed the add/storybook-for-components-2 branch from 13c79dc to f52cacd Compare October 4, 2019 10:14
@gziolo
Copy link
Member Author

gziolo commented Oct 4, 2019

I rebased changes against the add/storybook-for-components branch which was updated.

@gziolo
Copy link
Member Author

gziolo commented Oct 4, 2019

We should also explore how to setup Storybook for RN:
https://storybook.js.org/docs/guides/guide-react-native/

@epiqueras epiqueras force-pushed the add/storybook-for-components branch 2 times, most recently from 82498a8 to 2c47dcc Compare October 4, 2019 18:21
Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

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

It looks like this test depends on the execution time so it might fail from time to time. We probably should limit this check to whether this string starts with success message.

We should mock timers, I thought we already did.

We should also explore how to setup Storybook for RN:

For running on-device? Or are you proposing we have a second deployment that runs a web emulator?

bin/packages/build.js Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@epiqueras
Copy link
Contributor

@gziolo I've addressed the timing test issue in one of the Env follow ups in case there are some requirements that stop us from mocking timers: 43f1b23.

It'd be good to get those merged soon.

@gziolo
Copy link
Member Author

gziolo commented Oct 7, 2019

We should also explore how to setup Storybook for RN:

For running on-device? Or are you proposing we have a second deployment that runs a web emulator?

For running on the real device or the simulator. This is the only way to ensure that we build cross-platform components.

@gziolo gziolo force-pushed the add/storybook-for-components-2 branch from 524b3ad to 00ad822 Compare October 7, 2019 10:04
@gziolo gziolo requested a review from Soean as a code owner October 7, 2019 10:04
@gziolo gziolo changed the base branch from add/storybook-for-components to master October 7, 2019 10:04
@gziolo gziolo force-pushed the add/storybook-for-components-2 branch 3 times, most recently from 488d1ec to d564b55 Compare October 7, 2019 10:25
@epiqueras
Copy link
Contributor

For running on the real device or the simulator. This is the only way to ensure that we build cross-platform components.

I'll add it to my list.

@epiqueras
Copy link
Contributor

For running on the real device or the simulator. This is the only way to ensure that we build cross-platform components.

#17829

@gziolo gziolo force-pushed the add/storybook-for-components-2 branch from d564b55 to f9591bc Compare October 8, 2019 11:08
@@ -199,8 +199,9 @@ jobs:
if: branch = master
name: Deploy Playground
env: INSTALL_WORDPRESS=false
install:
Copy link
Member Author

Choose a reason for hiding this comment

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

How about we override install here to skip npm run build?

Copy link
Contributor

Choose a reason for hiding this comment

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

if: branch = master stops the entire stage from running outside of master, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see what you mean, because the playground already runs build. Yeah, good catch!

@epiqueras epiqueras merged commit b5c393a into master Oct 8, 2019
@epiqueras epiqueras deleted the add/storybook-for-components-2 branch October 8, 2019 17:55
@gziolo gziolo added the Storybook Storybook and its stories for components label Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Framework Issues related to broader framework topics, especially as it relates to javascript [Package] Components /packages/components Storybook Storybook and its stories for components [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants