Storybook for component development

[andrewn]

Integrates Storybook, one of the options discussed in #1216.

This will allow components to be developed in isolation for reuse across the Editor app.

I've also included styled-components as there needs to be some way to bundle the component styling with the component.

This should be reviewed against PR #1294.

Usage

• Check out this branch
• Run npm install
• Run npm run storybook
• Open the storybook web UI on http://localhost:9009/

Add-ons

• Docs: Displays a table of component props
• Actions: this allows click handlers to be logged in the Storybook "Action" tab
• Theme Playground: Allows theme to be toggled between Contrast, Dark and Light. Can also alter theme colors directly.
• Knobs: Dynamically alter props (e.g. button type, label etc) from Storybook UI

Example "Button" component

This PR includes a new Button component with associated "stories" - the name Storybook gives each example instance of the component with a set of props.

There are two ways of writing stories, and I've included an example of both for us to choose between.

A. Component Story Format (CSF)

File: client/components/Button/Button.stories.jsx

• Each story is exported as a named const from this file
• More structured, which I think's a good thing
• More familiar as it's all standard JS

B. MDX Format

Intro page: client/index.stories.mdx
File: client/components/Button/Button.stories.mdx

• Embed stories as JSX within a Markdown document
• Still need to import components
• More flexibility in how page is structured
• Allows markdown to be written around stories

Proposal

I'd actually propose that most of our stories should be in CSF format. If we need the extra flexibility of MDX then it's easy to swap to that.

[catarak]

I guess this also kicks off the process of migrating to Styled Components—exciting!

[andrewn]

@catarak, I've commented about organising stories.

I think the next steps are to implement that and properly extract the Button as a reusable component.

I'll continue in this PR?

[catarak]

@catarak, I've commented about organising stories.

I think the next steps are to implement that and properly extract the Button as a reusable component.

I'll continue in this PR?

sounds good!! i'm really excited to get this going ✨

[andrewn]

@catarak Ok, so this introduces a shared <Button /> component and replaces it across the app.

To avoid this becoming a long-running branch that diverges from master, I think we should aim to get this merged in first. Then I'll do small PRs that tackle different sections of the app.

[catarak]

Unfortunately a lot of the changes I've been working on for web accessibility are causing these merge conflicts.... i'm gonna try to fix them!

[catarak]

I've been working on the branch feature/storybook-cat! I made some pretty significant changes (that I'm not 100% sure of), so I figured I'd separate it in case you think another direction is better.

Basically, I decided to separate out the Icons from the Button component, because depending on how the icon is being used, you want it to have different accessibility attributes, that are separate from the function of the button. Sometimes the icon is purely decorative, and should be hidden from screen readers, and sometimes it should have a label.

With my latest commit f359dce, the Icon component is not totally finished, and maybe it doesn't even need to be to merge this in. It would be rad to by default make all of the components web accessibility friendly, which is what I tried to do with the withLabel HOC.

[andrewn]

I've been working on the branch feature/storybook-cat! I made some pretty significant changes (that I'm not 100% sure of), so I figured I'd separate it in case you think another direction is better.

I like this approach, it looks like it works well! I left a comment about naming the inputs and outputs of the HoC.

With my latest commit f359dce, the Icon component is not totally finished, and maybe it doesn't even need to be to merge this in.

What's left to do here? If it's enhancements—rather than fixing anything that's broken—then maybe open a PR from your branch to merge onto mine? We can continue to enhance as we expand the components.

I'm a big fan of trying to get small improvements regularly merged into the main project.

It would be rad to by default make all of the components web accessibility friendly, which is what I tried to do with the withLabel HOC.

Totally agree. Ideally, all the components will guide contributors (and us) to best practices by composing together in an accessible way and by having prop-types that require aria-labels etc.

[andrewn]

@catarak I thought about the approach in your latest commit some more and changed my mind...

The purpose of having iconBeforeName and iconAfterName is to ensure that the Button contents are properly layed out within the component and given appropriate spacing. Rather than passing in the icon as a child, using props:

• gives us more control of icon layout, we don't need to check every place the button is used to check weird combos or icons + elements
• allows us to show the different variations of buttons + icons in Storybooks
• makes the variations more discoverable via prop-types

How about we use iconBefore and iconAfter props to pass in the icon element to the button? This allows full control of the icon and allows us to have control of layout.

For example, this would become:

<Button
    iconAfter={<Icons.DropdownArrow />}
    onClick={() => setShowURL(!showURL)}
  >
    Share
</Button>

[catarak]

@catarak I thought about the approach in your latest commit some more and changed my mind...

The purpose of having iconBeforeName and iconAfterName is to ensure that the Button contents are properly layed out within the component and given appropriate spacing. Rather than passing in the icon as a child, using props:

• gives us more control of icon layout, we don't need to check every place the button is used to check weird combos or icons + elements
• allows us to show the different variations of buttons + icons in Storybooks
• makes the variations more discoverable via prop-types

How about we use iconBefore and iconAfter props to pass in the icon element to the button? This allows full control of the icon and allows us to have control of layout.

For example, this would become:

<Button
    iconAfter={<Icons.DropdownArrow />}
    onClick={() => setShowURL(!showURL)}
  >
    Share
</Button>

I agree with your reasons here! We also get Icons as their own component, and support control over different needs for web accessibility. I'll update the branch 😸

[catarak]

I just merged my branch with this one and made your suggested changes. Would you mind taking a look again? I couldn't decide whether to make common/Icons uppercase or lowercase, and tried lowercase but it looked weird to me, so I reverted to uppercase.

[andrewn]

Ok @catarak , this is looking good. Thanks for merging those changes!

Can you just check commit 161ac5b please? I've removed some aria-labels that duplicate button content. Or are misleading. I think that's ok?

On Icons vs icons, I think a file should be named after it's default export. But in this case we don't have one so I'm torn. import * as icons from "./icons" makes me think that icons contain a bunch of Icon components.

I'm happy with either though.

[catarak]

Can you just check commit 161ac5b please? I've removed some aria-labels that duplicate button content. Or are misleading. I think that's ok?

Thanks for catching those! You are right that they are unnecessary.

On Icons vs icons, I think a file should be named after it's default export. But in this case we don't have one so I'm torn. import * as icons from "./icons" makes me think that icons contain a bunch of Icon components.

I hear you! I want to go with what's clearest. I think I'm gonna change it.