Break up the component-library into smaller modules #1200

jaronheard · 2020-03-07T02:21:12Z

Summary

This pull request breaks up the component-library into smaller packages, with the intention of replacing and deprecating the component library in favor of these smaller modules.

It leaves the current component-library architecture as the default for now, so all components are now duplicated across packages. The plan for moving to the new components will be detailed in an upcoming RFC.

New packages

Packages	Description	Dependencies	Dev Dependencies	Comments
ui-brand	Brand assets	ui-themes, utils	ui-docs
ui-cards	Story cards	ui-brand*, ui-core, ui-themes, utils	ui-charts	ui-brand: Logo used for loading spinner
ui-charts	Charts components	ui-brand*, ui-themes, utils		ui-brand: Logo used for loading spinner
ui-core	Common UI components	ui-themes, utils
ui-docs	Style guide and Storybook styles	ui-themes	ui-brand
ui-maps	Map components	ui-brand, ui-charts, ui-core, ui-themes, utils
ui-themes	Themes and common styles
utils	Utilities

Next steps

Ensure that the packages are broken out in a way that makes sense
Add read-me to all packages, adjust documentation
Fix errors
Make it possible to run Storybook for component-library (yarn new-storybook)
Ensure that the correct version of Storybook is published to the web

Future work

Migration plan for component-library ([RFC] Component Library Migration Plan #1207)

jaronheard · 2020-05-17T17:10:56Z

Per discussion with @DingoEatingFuzz, making the following adjustments:

Removing ui-labs, in favor of developing within project packages and marking components experimental
Removing ui-platform in favor of developing within project package
Will check to see the size of ui-cards and ui-core? Could they bundled?

This reverts commit c8c3c5d.

ghost · 2020-05-25T23:34:40Z

I can't provide the caliber of technical review that @DingoEatingFuzz can, so I'll focus on participatory design aspects. These are probably the kinds of things that I'd put in an RFC about the migration process:

Should we transition all apps using the existing component library to this modular version and completely remove the old library? It would be ideal to preempt confusion about which library to use and I could see a newer dev thinking it's maybe easier to use the all-in-one-place-but-deprecated-and-enormous library. Alternatively, if migrating all apps using the deprecated library is hard, this could probably be accomplished with good documentation, but they'll need to be migrated over eventually anyways if we want them to benefit from component library updates (and keep our brand identity synced across apps)
I'm a bit concerned about having READMEs that just contain links to storybook because contents and instructions may be different for different versions. Also, they aren't even linked to specific storybook areas — perhaps those structures should line up? The READMEs should probably contain at least basic pertinent info and then link out to storybook with a message like "see the latest version and how to use it on storybook"

jaronheard · 2020-05-26T04:31:59Z

@Dianna, thanks for the review! In response to your bullets:

Should we transition all apps using the existing component library to this modular version and completely remove the old library?

See #1207 for details on your first bullet, and let me know if you have further comments after reviewing that.

I'm a bit concerned about the READMEs (sic)

Agreed, I'll flesh out the READMEs per your suggestion. I did the minimum viable READMEs as part of my first PR with the expectation that I'd fill them out a bit once we knew how everything was shaking out, so thanks for the reminder.

DingoEatingFuzz

This is looking really good!

I only had a couple things I wanted to ask about.

There are some duplicated files across packages. Are those intentional or by accident?
What's the deal with the .toMove files? Is that part of a multistep rollout or something?

Before this merges, I'd like to get some before and after package sizes as a comment on this PR for posterity.

Oh, also, like @Dianna said, the READMEs need to get better 😛 Deep-linking to parts of Storybook are a start. Using Docs mode within Storybook would make it even better, IMO. Even simple paragraphs that answer "why does this package exist" would be good.

I am okay with that all coming later though. Having PRs this large be open for this long is also not so good for participation (I say that knowing full well I am the problem there).

All in all great work and a huge undertaking!

DingoEatingFuzz · 2020-06-02T08:01:24Z

packages/ui-brand/mocha.conf.js

+
+// chai setup
+chai.use(sinonChai);
+chai.use(require("chai-enzyme")());


All this mocha stuff isn't necessary if the package doesn't have tests. There's probably also a good way to share this conf—or parts of it—with other packages.

DingoEatingFuzz · 2020-06-02T08:02:02Z

packages/ui-brand/package.json

+  "modules": "es/index.js",
+  "scripts": {
+    "build:esm": "rimraf es && babel src --out-dir es --copy-files --no-comments",
+    "build:cjs": "rimraf dist && babel src --out-dir dist --copy-files --no-comments",


It'd be nice to get to the point where we don't need to different build targets like this.

Will be addressed in #1208

DingoEatingFuzz · 2020-06-02T08:02:56Z

packages/ui-brand/package.json

+    "build:esm": "rimraf es && babel src --out-dir es --copy-files --no-comments",
+    "build:cjs": "rimraf dist && babel src --out-dir dist --copy-files --no-comments",
+    "build": "BABEL_ENV=esm yarn run build:esm && BABEL_ENV=cjs yarn run build:cjs",
+    "configure": "yarn run build",


I suspect this is copied from another package so I won't scrutinize but I think some of these scripts can be rethought and maybe trimmed back.

Will be addressed in #1208

DingoEatingFuzz · 2020-06-02T08:05:02Z

packages/ui-brand/scripts/start.js

@@ -0,0 +1,3 @@
+console.log(
+  "`yarn start` doesn't work here. Maybe you meant to run `yarn storybook` at project root?"
+);


If these commands are just going to log a line, you can use an echo directly in the package.json file.

"scripts": { "start": "echo '`yarn start` doesn\'t work here. Maybe you meant to run `yarn storybook` at the project root?'" }

Will be addressed in #1208

DingoEatingFuzz · 2020-06-02T08:11:03Z

packages/ui-charts/assets/civic-logo-animated-invert.svg

+		L328.1,122.7z"/>
+	<path class="st1" d="M0.1,0.1l10.5,10.5h317.5v112.1l10.5,10.3V0.1C338.6,0.1,0.1,0.1,0.1,0.1z"/>
+</g>
+</svg>


Do these SVGs also need to be here if they are in ui-brand? Is the intent to keep ui-charts from being bloated by superfluous ui-brand content?

Apparently there was a whole unused assets folder in ui-charts being useless...

DingoEatingFuzz · 2020-06-02T08:12:38Z

packages/ui-charts/assets/metadataQA.json

+    "whatisyourorganization": "Hack Oregon - 2019 Project Season",
+    "whoareyou": "Stephen Osserman, Nick Kobel, Xander Blair"
+  }
+}


Do these metadata json files need to be in both ui-cards and ui-charts?

Apparently there was a whole unused assets folder in ui-charts being useless...

DingoEatingFuzz · 2020-06-02T08:18:42Z

packages/ui-core/src/ButtonNew/ButtonNew.js

+import MaterialButton from "@material-ui/core/Button";
+
+import { ThemeProvider } from "@material-ui/styles";
+import { MaterialTheme } from "@hackoregon/ui-themes";


Have you run numbers yet on how large ui-core is now? I'm curious how much weight material, emotion, and our ui themes adds.

Could this be right?

This whole measuring things ad-hoc is a little problematic. I'd like to put in some automated checks for bundle size and performance.

jaronheard · 2020-06-15T05:24:30Z

There are some duplicated files across packages. Are those intentional or by accident?

The duplicated files that you identified was accidental. I've removed the extraneous asset folder that existed in ui-charts

What's the deal with the .toMove files? Is that part of a multistep rollout or something?

Yep, rather than update 2018 to use the new components now, I'm going to do that in a separate PR. The components that will live within 2018 are in a temporary .toMove folder, and will live within src in the future.

The tarball size comparison[1]:

For some reason Bundlephobia fails on the component-library ... I was able to get a direct comparison using this tool.

jaronheard · 2020-06-15T06:32:11Z

Ok, I think this is ready now -- the comments have either been resolved, or incorporated into #1207.

jaronheard added 24 commits March 4, 2020 18:29

add ui-maps and ui-not-maps packages

9875c69

remove maps from notmaps

2f76624

remove non-maps from ui-maps

bfb5723

demonstrate storybook working across packages

da7cf85

create ui-theme

b1e2be3

wip added ui-charts, not connected

4903f13

rename to ui-themes

7f98de0

ui-core, ui-cards

8c5e5d2

update package jsons

dbb8e99

update versions

6feae13

add isClient export

9a0f731

update versions

19e4d57

update linting errors

e6fa8ac

update version

7ec496d

remove assets from ui-cards

34a2d64

remove assets from ui-core

8d3bbae

remove assets from ui-maps

94788e2

factor out utils to separate files

57cd8a1

storybook working

896be4f

add no side-effects to modules

4acab0c

storybook working and sorted

4d1df47

add lab package

4e4744b

0.0.4

3df8eb2

add updated brand theme

230e658

jaronheard changed the title ~~Component library anti trust~~ Break up the component-library into smaller modules Mar 7, 2020

jaronheard added 5 commits April 30, 2020 21:19

update package readmes

e1b825d

update lockfile

88a5a43

limit lerna version to master

3240e84

fix error in CardList

415663b

add storybook-new command

eb93e48

jaronheard mentioned this pull request May 6, 2020

[RFC] Component Library Migration Plan #1207

Open

jaronheard added 8 commits May 17, 2020 12:32

upgrade storybook, storybook webpack broken

966e658

update webpack config to fix build

b3d73aa

Run add-component-parameters on ui- packages

c8c3c5d

Revert "Run add-component-parameters on ui- packages"

6abf74a

This reverts commit c8c3c5d.

move ui-platform to 2018 without adding dependencies

c3b2e07

Move story dependencies to devDependencies

6921127

adjust internal imports for ui-maps and other packages`

f41c7e6

move ui-platform to toMove

9e84811

jaronheard requested review from a user and DingoEatingFuzz May 25, 2020 20:54

DingoEatingFuzz reviewed Jun 2, 2020

View reviewed changes

remove assets from ui-charts

7d0ca8d

This was referenced Jun 15, 2020

Common module build script and tooling clean-up #1208

Closed

Common module documentation improvement #1209

Open

jaronheard merged commit 8bf84ec into master Jul 9, 2020

jaronheard deleted the component-library-anti-trust branch July 9, 2020 16:16

jaronheard mentioned this pull request Apr 6, 2021

Update component library hackoregon/openelections#1139

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break up the component-library into smaller modules #1200

Break up the component-library into smaller modules #1200

jaronheard commented Mar 7, 2020 •

edited

Loading

jaronheard commented May 17, 2020 •

edited

Loading

ghost commented May 25, 2020

jaronheard commented May 26, 2020 •

edited

Loading

DingoEatingFuzz left a comment

DingoEatingFuzz Jun 2, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

DingoEatingFuzz Jun 2, 2020

jaronheard Jun 15, 2020

jaronheard Jun 15, 2020

jaronheard Jun 15, 2020

jaronheard commented Jun 15, 2020 •

edited

Loading

jaronheard commented Jun 15, 2020

Break up the component-library into smaller modules #1200

Break up the component-library into smaller modules #1200

Conversation

jaronheard commented Mar 7, 2020 • edited Loading

Summary

New packages

Next steps

Future work

jaronheard commented May 17, 2020 • edited Loading

ghost commented May 25, 2020

jaronheard commented May 26, 2020 • edited Loading

DingoEatingFuzz left a 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

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

jaronheard commented Jun 15, 2020 • edited Loading

jaronheard commented Jun 15, 2020

jaronheard commented Mar 7, 2020 •

edited

Loading

jaronheard commented May 17, 2020 •

edited

Loading

jaronheard commented May 26, 2020 •

edited

Loading

jaronheard commented Jun 15, 2020 •

edited

Loading