Skip to content
This repository has been archived by the owner on Dec 6, 2022. It is now read-only.

Add guide for writing stories #903

Merged
merged 17 commits into from
Aug 16, 2021
Merged

Add guide for writing stories #903

merged 17 commits into from
Aug 16, 2021

Conversation

jamie-lynch
Copy link

@jamie-lynch jamie-lynch commented Aug 3, 2021

What is the purpose of this change?

This PR adds a document which sets out a guide for writing stories to help ensure consistency and best practice.

Background

This document is being added during the migration of API docs from theguardian.design into Storybook. I have written this up whilst working on #902 so that PR is a good example of the guidelines in action.

I'm adding it now so that as we continue with the migration and find even more ways to improve the documentation we can record these explicitly for the future, but also so that we can have the discussion around best practice for stories in one place, rather than split across multiple PRs throughout the migration.

@jamie-lynch jamie-lynch requested a review from a team as a code owner August 3, 2021 08:33
docs/10-stories.md Outdated Show resolved Hide resolved
docs/10-stories.md Outdated Show resolved Hide resolved
docs/10-stories.md Outdated Show resolved Hide resolved
docs/10-stories.md Outdated Show resolved Hide resolved
Jamie Lynch and others added 2 commits August 3, 2021 11:04
docs/10-stories.md Outdated Show resolved Hide resolved
};

export const NoValue = (args: MyComponentProps) => (
<MyComponent {...args} label="Label"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not passing default args via the NoValue.args = {...} pattern, is it useful to spread the args into the component like this? Is it doing anything?

Copy link
Author

Choose a reason for hiding this comment

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

It's not, good point. The only thing that we need is to pass an arg into the function so that Storybook generates the source for the docs page. We could instead do:

export const NoValue = (_: MyComponentProps) => (
  <MyComponent label="Label"/>
)

Copy link
Contributor

@SiAdcock SiAdcock Aug 3, 2021

Choose a reason for hiding this comment

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

Oh wow, that's unusual. So if you don't specifically define an argument, the docs don't get generated? You couldn't simply do:

export const NoValue = () => (
  <MyComponent label="Label"/>
)

?

Copy link
Author

Choose a reason for hiding this comment

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

Correct - for example:

With Args

export const LegendSupportingTextLight = (args: RadioGroupProps) => (
	<RadioGroup
		{...args}
		name="colours"
		label="Select your preferred colour"
		supporting="You can always change it later"
	>
		{radiosOneSelected.map((radio, index) =>
			React.cloneElement(radio, { key: index }),
		)}
	</RadioGroup>
);

or

export const LegendSupportingTextLight = (_: RadioGroupProps) => (
	<RadioGroup
		name="colours"
		label="Select your preferred colour"
		supporting="You can always change it later"
	>
		{radiosOneSelected.map((radio, index) =>
			React.cloneElement(radio, { key: index }),
		)}
	</RadioGroup>
);

image

Without Args

export const LegendSupportingTextLight = () => (
	<RadioGroup
		name="colours"
		label="Select your preferred colour"
		supporting="You can always change it later"
	>
		{radiosOneSelected.map((radio, index) =>
			React.cloneElement(radio, { key: index }),
		)}
	</RadioGroup>
);

image

Copy link
Member

Choose a reason for hiding this comment

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

storybook must be able to introspect useful info from you explicitly giving the args type, even if you don't use it?

Copy link
Author

Choose a reason for hiding this comment

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

Are we happy with this then?

export const LegendSupportingTextLight = (_: RadioGroupProps) => (
	<RadioGroup
		name="colours"
		label="Select your preferred colour"
		supporting="You can always change it later"
	>
		{radiosOneSelected.map((radio, index) =>
			React.cloneElement(radio, { key: index }),
		)}
	</RadioGroup>
);

docs/10-stories.md Outdated Show resolved Hide resolved
docs/10-stories.md Outdated Show resolved Hide resolved
Jamie Lynch and others added 2 commits August 5, 2021 13:13
Co-authored-by: Alex Sanders <alex@sndrs.dev>
@sndrs
Copy link
Member

sndrs commented Aug 5, 2021

how about a rule where we don't use emotion in stories? if you need to style things, you can use the style prop, but maybe best is show things unstyled? that's the raw component being demoed then. e.g.

image

@jamie-lynch
Copy link
Author

how about a rule where we don't use emotion in stories? if you need to style things, you can use the style prop, but maybe best is show things unstyled? that's the raw component being demoed then. e.g.

image

Hmm... I thought I agreed with this but then I had a play with the Link stories and I'm not sure. I think there are good cases for using styles to improve the story but I'm not sure that having style objects in the rendered output is a good thing. For starters, it makes it easy to copy and paste something that (I don't think) we'd recommend.

image

@sndrs
Copy link
Member

sndrs commented Aug 5, 2021

is it useful though to style the demo? i feel like that should show us what you get out the box, rather than an example of using it?

@jamie-lynch
Copy link
Author

is it useful though to style the demo? i feel like that should show us what you get out the box, rather than an example of using it?

Yeah, maybe for the Demo it should always just be the component on it's own. I'll update the recommendations to include that.

I'd argue that some of the styling is useful for stories that are used for visual regression testing as it can test them in context or test multiple instances of the component in one story. I've posted a couple more ideas for how to improve those code blocks in the Improve storybook code blocks card on the backlog. Plus we can add a recommendation to minimise styling in stories.

@sndrs
Copy link
Member

sndrs commented Aug 6, 2021

that's a good point – maybe we just don't want code for the chromatic examples? only the demo with the knobs?

Copy link
Member

@sndrs sndrs left a comment

Choose a reason for hiding this comment

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

love it!

@jamie-lynch jamie-lynch merged commit 245fc13 into main Aug 16, 2021
@jamie-lynch jamie-lynch deleted the stories-docs branch August 16, 2021 14:43
@SiAdcock
Copy link
Contributor

Amazing work, thanks @sndrs and @jamie-lynch 💎

sndrs added a commit that referenced this pull request Aug 17, 2021
* Add doc with best practice for story configuration

* Update docs/10-stories.md

Co-authored-by: Alex Sanders <alex@sndrs.dev>

* include Editorial in stories title pattern

* Update docs/10-stories.md

Co-authored-by: Simon Adcock <simonadcock2@gmail.com>

* standardise configuration approach

* update story file name recommendation

* add stories doc link to README

* Add note about using ignore to remove args completely

* update example to match updated recommendations

* Update docs/10-stories.md

Co-authored-by: Alex Sanders <alex@sndrs.dev>

* add recommendations about styling

* update recommendations

* replace tabs with spaces

* update the string | JSX.Element tip

Co-authored-by: Alex Sanders <alex@sndrs.dev>
Co-authored-by: Simon Adcock <simonadcock2@gmail.com>
@mxdvl mxdvl mentioned this pull request Oct 6, 2021
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants