WIP - Storybook UI Library #3087

codemist · 2023-05-30T19:48:14Z

References:

Description

Add Storybook (https://storybook.js.org/) to project to view components/design system elements. This includes a Button element, Status Pill and the Exposure Card to test.

Screenshot (if applicable)

How to test

Run npm run storybook to preview.

Checklist (Definition of Done)

Localization strings (if needed) have been added.
Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
I've added a unit test to test for potential regressions of this bug.
Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

src/app/(nextjs_migration)/components/client/button.module.scss

src/app/(nextjs_migration)/components/client/Button.stories.ts

src/app/(nextjs_migration)/components/client/button.module.scss

src/app/package.json

pdehaan · 2023-05-30T21:38:53Z

src/app/storybook.log

+npm ERR! network 'proxy' config is set properly.  See: 'npm help config'
+
+npm ERR! A complete log of this run can be found in:
+npm ERR!     /Users/kandres/.npm/_logs/2023-05-30T16_33_34_119Z-debug-0.log


We probably didn't mean to commit this file.

src/app/tokens.scss

.storybook/main.ts

.storybook/preview.ts

pdehaan · 2023-05-31T21:31:56Z

src/app/(nextjs_migration)/components/client/Button.tsx

+import React from "react";
+import styles from "./button.module.scss";
+
+export type Props = {


Q: Does anything import this type? Does it need to be exported?
If so, does it need a less conflicting name, like ButtonProps, or should the importer just alias it?

Unless these are just temporary placeholder components.

Usually you won't need to import Prop types, but when one component passes on props to another, it can be useful. So I also tend to export them by default, but use the generic name Props to avoid having another thing to rename when copy-pasting an existing file 😅 Potential importers can then indeed just alias them.

pdehaan · 2023-05-31T21:33:07Z

src/app/(nextjs_migration)/components/client/Button.tsx

+  const classes = [
+    styles.button,
+    styles[type],
+    destructive && styles.destructive,
+    large && styles.large,
+  ]
+    .filter(Boolean)
+    .join(" ");


Not sure if something like https://www.npmjs.com/package/classnames would simplify this.
I see they have some sort of support for CSS-modules too.

Will look into this for future iterations.

pdehaan · 2023-05-31T21:33:45Z

src/app/(nextjs_migration)/components/client/StatusPill.tsx

+import React from "react";
+import styles from "./StatusPill.module.scss";
+
+export type Props = {


Same Q here, re: export and or StatusPillProps.

pdehaan · 2023-05-31T22:00:41Z

src/app/(nextjs_migration)/components/client/StatusPill.module.scss

+.pill {
+  font: $text-body-sm;
+  font-weight: 600;
+  background: black;


Suggested change

background: black;

background: $color-black;

pdehaan · 2023-05-31T22:01:14Z

src/app/(nextjs_migration)/components/client/button.module.scss

+  line-height: 1;
+
+  &.primary {
+    color: white;


Suggested change

color: white;

color: $color-white;

pdehaan · 2023-05-31T22:01:29Z

src/app/(nextjs_migration)/components/client/button.module.scss

+
+  &.large {
+    font: $text-body-lg;
+    color: white;


Suggested change

color: white;

color: $color-white;

pdehaan · 2023-05-31T22:03:04Z

src/app/(nextjs_migration)/components/client/button.module.scss

+    color: $color-blue-50;
+    border: solid 2px $color-blue-50;
+    background-color: transparent;
+    box-shadow: rgba(0, 0, 0, 0.15) 0px 0px 0px 1px inset;


Suggested change

box-shadow: rgba(0, 0, 0, 0.15) 0px 0px 0px 1px inset;

box-shadow: rgba($color-black, 0.15) 0 0 0 1px inset;

src/app/.storybook/preview.js

pdehaan · 2023-06-06T16:49:23Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+        <dt>{headline}</dt>
+        <dl>{description}</dl>


I dont think this is semantically correct.
IIUC, per https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dl, it should be:

<dl> <dt>Firefox</dt> <dd> A free, open source, cross-platform, graphical web browser developed by the Mozilla Corporation and hundreds of volunteers. </dd>  <dt>...</dt> <dd>...</dd> </dl>

So not sure if this should be:

<dl> <dt>{headline}</dt> <dd>{description}</dd> </dl>

Although that just always creates a description list with one list item. Or if it just returns this (like you had, but with <dd> instead of <dl>) and then the parent container wraps it in a <dl> somewhere:

<> <dt>{headline}</dt> <dd>{description}</dd> </>

pdehaan · 2023-06-06T19:37:22Z

src/app/fonts/inter.scss

+/* Arial font-size matching for minimal CLS */
+
+@font-face {
+  font-family: Inter-fallback;


Not relevant to much probably, but I was reading through the Next.js docs the other day and came across this:
https://nextjs.org/docs/pages/building-your-application/optimizing/fonts

import { Inter } from 'next/font/google'; // If loading a variable font, you don't need to specify the font weight const inter = Inter({ subsets: ['latin'] });

Not sure if that's worth filing a separate ticket/spike for to investigate in the future if we could potentially do font optimizing.

I also didn't see a Metropolis font on Google Fonts, so wasnt sure how to do potential font optimizing for locally hosted fonts.

Yeah, and AFAIK we're already using that - Storybook is useful, but it shouldn't negatively impact what our actual website looks like. So if Storybook has issues loading the fonts, let's see if we can fix that in a way that doesn't require changes to the actual website code - and until we've found such a fix, we'll just have to accept that the fonts don't look quite right there yet.

We may not be able to take advantage of the Google Font optimization for Inter in Next.js due to our current handling of tokened fonts for Metropolis. Having two different approaches to loading the fonts could lead to confusion.

maxxcrawford · 2023-06-06T20:38:28Z

.storybook/preview.ts

+      },
+    },
+    status: {
+      type: "beta", // 'beta' | 'stable' | 'deprecated' | 'releaseCandidate'


Note: This currently applies to ALL components. We need to figure out how to set individual status levels.

You should be able to add parameters to individual stories. That said, I'm not sure if we need a heavyweight release process for these components; since we're the only consumer, I'd rather do without the overhead and just consider them evergreen, at least initially.

maxxcrawford · 2023-06-06T20:39:13Z

I think we can review this as is. I added a "status" label add-on so that we can caveat which components are completed/etc.

@pdehaan @codemist @Vinnl

pdehaan · 2023-06-06T22:13:31Z

.storybook/main.ts

+import type { StorybookConfig } from "@storybook/nextjs";
+
+const config: StorybookConfig = {
+  stories: ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|ts|tsx)"],


Tiny nit: do we need all these globs, or can we standardize on using .ts and/or .tsx files?

git ls-files src/**/*.stories.* src/app/(nextjs_migration)/components/client/stories/Button.stories.ts src/app/(nextjs_migration)/components/client/stories/ExposureCard.stories.ts src/app/(nextjs_migration)/components/client/stories/StatusPill.stories.ts

We'll probably at least want the .mdx as well, for more documentation-style stories (though possibly we want those to be .stories.mdx as well?). Though I also don't really mind covering our bases and supporting JS stories as well.

pdehaan · 2023-06-06T22:23:03Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+  OpenInNew,
+  PhoneIcon,
+} from "./Icons";
+import Image from "next/image";


Merge w/ L8 above?

pdehaan · 2023-06-06T22:26:45Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+import { Button } from "./Button";
+import Email from "next-auth/providers/email";
+
+export type Props = {


Same as above (or below, or somewhere), should this be renamed to "ExposureCardProps" (not sure if anybody is importing these props or not)

pdehaan · 2023-06-06T22:30:13Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+export type Props = {
+  exposureImg: StaticImageData;
+  exposureName: string;
+  exposureType: string;


re: "*Type" suffix should these be generic string; or enums?
(Same w/ L27 below for the statusPillType declaration)

pdehaan · 2023-06-06T22:31:25Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+  const DetailsFoundItem = (props: DetailsFoundProps) => {
+    let headline, description;
+    if (props.whichExposed === "family") {
+      headline = "Family members";


Not sure if we want to add TODO statements to refactor all this for l10n-ing.

pdehaan · 2023-06-06T22:34:15Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+          <span className={styles.exposureTypeIcon}>{props.icon}</span>
+          {headline}
+        </dt>
+        <dl>{description}</dl>


IIUC, i think <dl> is supposed to be a parent of <dt> and <dd> tags

pdehaan · 2023-06-06T22:35:58Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+  const elementCard = (
+    <div>
+      <div className={styles.exposureCard}>
+        <div className={styles.exposureHeader}>


nit: should this be <div> or <header>?

pdehaan · 2023-06-06T22:37:12Z

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

+    );
+  };
+  const elementCard = (
+    <div>


nit: Do we need this wrapper div, or can we just use the child <div className={styles.exposureCard}> as the top level container?

pdehaan · 2023-06-06T22:49:28Z

src/app/.storybook/main.js

+const config = {
+  stories: [
+    // "../components/client/stories/**/*.mdx",
+    "../components/client/stories/**/*.stories.@(js|jsx|ts|tsx)",


Do we need all these globs, or can we reduce this to just .ts and/or .tsx?

Vinnl · 2023-06-07T12:14:53Z

A wild preview deployment appeared!

Vinnl

Really cool that this adds all the Storybook infra, this is going to be massively useful! Needs a bit of cleanup, but let's do that, then merge this in, and then we can work on/refine individual components in followup pull requests as needed. To clean up to merge, I think we should:

Revert the addition of unoptimised Inter
Remove the second .storybook folder
Move new components out of /app/(nextjs_migration), e.g. to `/app/components/

Nice work!

Vinnl · 2023-06-07T12:28:10Z

.storybook/main.ts

+import type { StorybookConfig } from "@storybook/nextjs";
+
+const config: StorybookConfig = {
+  stories: ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|ts|tsx)"],


We'll probably at least want the .mdx as well, for more documentation-style stories (though possibly we want those to be .stories.mdx as well?). Though I also don't really mind covering our bases and supporting JS stories as well.

Vinnl · 2023-06-07T12:35:17Z

.storybook/preview.ts

+      },
+    },
+    status: {
+      type: "beta", // 'beta' | 'stable' | 'deprecated' | 'releaseCandidate'


You should be able to add parameters to individual stories. That said, I'm not sure if we need a heavyweight release process for these components; since we're the only consumer, I'd rather do without the overhead and just consider them evergreen, at least initially.

Vinnl · 2023-06-07T12:37:23Z

src/app/(nextjs_migration)/components/client/Button.tsx

+import React from "react";
+import styles from "./button.module.scss";
+
+export type Props = {


Usually you won't need to import Prop types, but when one component passes on props to another, it can be useful. So I also tend to export them by default, but use the generic name Props to avoid having another thing to rename when copy-pasting an existing file 😅 Potential importers can then indeed just alias them.

Vinnl · 2023-06-07T12:38:43Z

src/app/(nextjs_migration)/components/client/Button.tsx

+import styles from "./button.module.scss";
+
+export type Props = {
+  type: string; // primary | secondary | tertiary


You can actually encode this using a union (|) of literal types (e.g. "literalString", or 1, or false) 😁

Suggested change

type: string; // primary | secondary | tertiary

type: "primary" | "secondary" | "tertiary";

Same for a couple of props in ExposureCard.

src/app/(nextjs_migration)/components/client/Button.tsx

src/app/(nextjs_migration)/components/client/ExposureCard.module.scss

src/app/(nextjs_migration)/components/client/ExposureCard.tsx

src/app/(nextjs_migration)/components/client/Icons.tsx

src/app/.storybook/main.js

Vinnl · 2023-06-07T13:00:34Z

src/app/fonts/inter.scss

+/* Arial font-size matching for minimal CLS */
+
+@font-face {
+  font-family: Inter-fallback;


Yeah, and AFAIK we're already using that - Storybook is useful, but it shouldn't negatively impact what our actual website looks like. So if Storybook has issues loading the fonts, let's see if we can fix that in a way that doesn't require changes to the actual website code - and until we've found such a fix, we'll just have to accept that the fonts don't look quite right there yet.

add template button component

773e61e

codemist changed the base branch from main to nextjs May 30, 2023 19:50

codemist added 2 commits May 30, 2023 21:51

remove assets

f7922af

add button styling

f68a9fc

pdehaan reviewed May 30, 2023

View reviewed changes

codemist added 2 commits May 31, 2023 22:20

use old font code and add status pill component

de6521d

remove unnecessary package additions and style status pills

bd5a567

codemist force-pushed the storybook-ui branch from ec4f840 to bd5a567 Compare May 31, 2023 20:45

pdehaan reviewed May 31, 2023

View reviewed changes

lint

f55eeee

codemist force-pushed the storybook-ui branch from cd7a51d to f55eeee Compare June 1, 2023 15:58

test exposurecard data func

ba3d463

codemist force-pushed the storybook-ui branch from fe1ba97 to ba3d463 Compare June 4, 2023 21:31

This comment was marked as spam.

Sign in to view

add toggle to exposurecard accordion

7a6430a

codemist force-pushed the storybook-ui branch from 1c1244d to 7a6430a Compare June 6, 2023 15:30

pdehaan reviewed Jun 6, 2023

View reviewed changes

add icons to exposure type

b41443b

maxxcrawford force-pushed the storybook-ui branch 3 times, most recently from d6c6942 to 27c12b1 Compare June 6, 2023 20:26

maxxcrawford reviewed Jun 6, 2023

View reviewed changes

maxxcrawford marked this pull request as ready for review June 6, 2023 20:39

maxxcrawford requested a review from Vinnl June 6, 2023 20:39

pdehaan reviewed Jun 6, 2023

View reviewed changes

mozilla deleted a comment from netlify bot Jun 7, 2023

Vinnl reviewed Jun 7, 2023

View reviewed changes

remove introduction.mdx for now, refine button states

18482ed

apply some changes

e9a6e75

codemist force-pushed the storybook-ui branch from fedde06 to e9a6e75 Compare June 8, 2023 09:14

Vinnl and others added 7 commits June 8, 2023 11:25

Move new components out of migration dir

1962284

Merge branch 'nextjs' into storybook-ui

86a5c4d

Delete .bash_profile

bac2759

Delete storybook.log

8cd139a

Delete main.js

eaf426a

Delete preview.js

84a012f

remove use of inter for now

ba74e39

Vinnl approved these changes Jun 8, 2023

View reviewed changes

codemist merged commit 85c5c8a into nextjs Jun 8, 2023

codemist deleted the storybook-ui branch June 8, 2023 14:48

	box-shadow: rgba(0, 0, 0, 0.15) 0px 0px 0px 1px inset;
	box-shadow: rgba($color-black, 0.15) 0 0 0 1px inset;

	type: string; // primary \| secondary \| tertiary
	type: "primary" \| "secondary" \| "tertiary";

WIP - Storybook UI Library #3087

WIP - Storybook UI Library #3087

Conversation

codemist commented May 30, 2023 • edited Loading

References:

Description

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxxcrawford commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl commented Jun 7, 2023

Vinnl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemist commented May 30, 2023 •

edited

Loading

Vinnl left a comment •

edited

Loading