feat(icons): delete old icons, add new streamline icons #129

TheSisb · 2019-10-09T23:40:35Z

Our icons package needed some serious love. This PR is massive, here's what happened:

High level user-facing stuff:

Nuked all our old inherited icons. We're starting with a fresh slate using Streamline icons.
I added two new icons (thx @serifluous): PlusIcon and LoadingIcon.
Updated the Spinner and Button components to use our new LoadingIcon.
Our icons now are typed correctly with our tokens. No more arbitrary number passing (in TS)!
Our icon size tokens have been updated to 16px 24px and 32px
I updated the Spinner story: it now uses tokens and has a responsive demo.
I made our icons package npm publishable again by adding the missing publishConfig.
Note: Color is now passed as iconColor to icons, rather than simply color. code
Our TextColor typing now also accepts currentColor. code

Icons package janitorial work

IconWrapper is cleaned up and now uses tokens.
Previously, we had these folders: src/svg and src/react. I moved the raw SVGs out of src, so it's just svg, and moved react directly into src. This helped consolidate this package's architecture to match other packages in the monorepo.
I swapped out tsc for rollup to run our icon builds. This along with some of the other tweaks helped to eliminate the monorepo issue where built files would be generated into incorrect directories.
Rollup by default bundles all the icons into a single index file. We don't want consumers to have to download a massive bundle of icons if they're only using a handful. In the rollup config, we have to explicitly specificy each input file if we want them to be separate. Rather than updating it by hand each time we add a new icon, I generate a cache of icons in the system during the list-icons step. This file is commit to the repo so builds work for everyone.
A whole slew of cleanup to our icons package build scripts and tooling. Mostly refactoring to make it more robust and easier to read.

Other infra work

I added an iconSizes export from our token page, for easier typing. code
Our core-bundle kept generating a diff every rebuild, so I added sorting to the generated dependencies and imports. This removes any meaningless changes when running builds because the output stays consistent.
A small thing, but I noticed all our rollup files were declaring format as es instead of esm, so I updated them all to say esm which matches the rollup docs.

Future work

I'd like to add more documentation to this package, to make it easier to navigate for future maintainers.

vercel · 2019-10-09T23:40:41Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/twilio-dsys/paste/ed0z6p0wo
🌍 Preview: https://paste-git-icons-migrate-to-streamline.twilio-dsys.now.sh

SiTaggart · 2019-10-10T01:09:33Z

The spinner isn't perfectly centered so it moves around slightly by like 2 pixels

Can you add an example of loading buttons to the button docs too with this?

SiTaggart · 2019-10-10T01:18:51Z

Should we change the PR title to be BREAKING CHANGE: ... too, as the merge commit needs to be that for lerna to do a major bump?

TheSisb · 2019-10-10T19:32:16Z

Should we change the PR title to be BREAKING CHANGE: ... too, as the merge commit needs to be that for lerna to do a major bump?

I was thinking about that on my Box PR as well. Github topics can only be a single line though. Conventional commits dictates that the title be "fix/etc" and "BREAKING CHANGE" be placed on another line beneath.

Looking at how my Box PR merged, it looks like it worked itself out:

If I hit "Squash and merge" on this PR it also does the correct thing:

One takeaway is that we should isolate BREAKING CHANGES is in PRs so that unrelated packages don't get a major bump for nothing.

richbachman · 2019-10-10T19:33:56Z

Just echoing the off center spinner here. Also, more of a visual thing, but I feel like the weight of the icons is off, especially when used alongside text:

serifluous · 2019-10-11T18:14:55Z

Just echoing the off center spinner here. Also, more of a visual thing, but I feel like the weight of the icons is off, especially when used alongside text:

After working with @TheSisb, we realized the 1px stroke width means centered icons like the plus icon don't align to full pixels. We're upping icon strokes to 2px.

TheSisb · 2019-10-15T21:40:31Z

@SiTaggart @serifluous @richbachman Ready for re-review.

serifluous · 2019-10-16T16:41:01Z

I can't see any of the changes. Can you add the loading button example to the website?

TheSisb · 2019-10-16T19:21:13Z

I can't until we fix another issue with the release icons package on NPM. For now the only way to test is to pull this locally and run storybook :(

serifluous · 2019-10-16T19:27:28Z

I’ll try pulling it locally. But don’t let that hold this up. It looked good to me when we looked at it together yesterday.

SiTaggart · 2019-10-17T15:50:44Z

super crispy 👌

packages/paste-core/components/spinner/src/index.tsx

BREAKING CHANGE: removed all the inherited icons since we're moving to a new system

- huge overhaul to build process - now uses rollup - builds into cjs and esm directories - made the package publishable to npm - moved svg folder out of src, now src only holds react stuff - updated scripts to use new paths and cleaned up the code - programmatically generates rollup config from the icon-list command

richbachman · 2019-11-05T16:56:39Z

This looks good to me. Nice job!

packages/paste-core/components/spinner/src/index.tsx

TheSisb force-pushed the icons/migrate-to-streamline branch from 8a5b517 to bbdee6d Compare October 9, 2019 23:52

vercel bot deployed to staging October 9, 2019 23:53 View deployment

TheSisb requested review from richbachman and SiTaggart October 10, 2019 00:03

TheSisb self-assigned this Oct 10, 2019

TheSisb added Area: Components Related to the component library (core) of this system Area: Storybook Related to Storybook Status: Pls CR This PR is ready for Code Reviews Type: Bug Something isn't working Type: Enhancement Changes that don't affect the meaning of the code labels Oct 10, 2019

vercel bot deployed to staging October 10, 2019 00:08 View deployment

vercel bot deployed to staging October 11, 2019 15:35 View deployment

TheSisb force-pushed the icons/migrate-to-streamline branch from ba83e8c to 5f4e98d Compare October 15, 2019 18:15

vercel bot deployed to staging October 15, 2019 18:15 View deployment

SiTaggart reviewed Oct 17, 2019

View reviewed changes

packages/paste-core/components/spinner/src/index.tsx Outdated Show resolved Hide resolved

vercel bot deployed to staging October 17, 2019 21:00 View deployment

vercel bot had a problem deploying to staging October 22, 2019 23:13 Failure

TheSisb force-pushed the icons/migrate-to-streamline branch from f3badba to b9b3b09 Compare October 23, 2019 22:21

vercel bot deployed to staging October 23, 2019 22:21 View deployment

TheSisb added 20 commits November 4, 2019 17:04

feat(icons): delete old icons, add new streamline icons

6e2aefe

BREAKING CHANGE: removed all the inherited icons since we're moving to a new system

chore(icons): update icon list for storybook

2a6c07e

fix(spinner): use new icon

957d49c

fix(storybook): use new icon for story

4231093

fix(story): button icons should be 24px

9d5b3d5

fix: adjust icon size tokens

1d34c5e

feat: add iconSizes as separate key in theme-tokens

de92311

feat(icons): icons package to use tokens and update icons

9bea84c

fix(spinner): update to use tokens and new icons

d8445b5

fix(button): use correct spinner size

1568c16

chore: add new icons dist folders to gitignore

5e36980

fix: spinner and button icon usage

d5460c0

feat(icons): add rollup icon list cache file

3f0d110

fix(core-bundle): sort packages for consistent builds

92bf145

chore: use esm instead of es in rollup for correctness

cc543d3

chore: yarn.lock

bc7004e

fix(spinner): lint error

8491fe1

chore(icons): move rollup icon list

d7e75d4

chore(spinner): use types package in story

bc746d6

TheSisb force-pushed the icons/migrate-to-streamline branch from d920c3e to bc746d6 Compare November 4, 2019 23:07

vercel bot deployed to staging November 4, 2019 23:07 View deployment

richbachman approved these changes Nov 5, 2019

View reviewed changes

SiTaggart reviewed Nov 5, 2019

View reviewed changes

packages/paste-core/components/spinner/src/index.tsx Outdated Show resolved Hide resolved

fix(spinner): swap out destructured props for explicit props

ebb040a

vercel bot deployed to staging November 5, 2019 18:31 View deployment

SiTaggart approved these changes Nov 5, 2019

View reviewed changes

TheSisb merged commit 571791d into master Nov 5, 2019

TheSisb deleted the icons/migrate-to-streamline branch November 5, 2019 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icons): delete old icons, add new streamline icons #129

feat(icons): delete old icons, add new streamline icons #129

TheSisb commented Oct 9, 2019 •

edited

Loading

vercel bot commented Oct 9, 2019 •

edited

Loading

SiTaggart commented Oct 10, 2019

SiTaggart commented Oct 10, 2019

TheSisb commented Oct 10, 2019 •

edited

Loading

richbachman commented Oct 10, 2019

serifluous commented Oct 11, 2019

TheSisb commented Oct 15, 2019

serifluous commented Oct 16, 2019

TheSisb commented Oct 16, 2019 •

edited

Loading

serifluous commented Oct 16, 2019

SiTaggart commented Oct 17, 2019 •

edited

Loading

richbachman commented Nov 5, 2019

feat(icons): delete old icons, add new streamline icons #129

feat(icons): delete old icons, add new streamline icons #129

Conversation

TheSisb commented Oct 9, 2019 • edited Loading

High level user-facing stuff:

Icons package janitorial work

Other infra work

Future work

vercel bot commented Oct 9, 2019 • edited Loading

SiTaggart commented Oct 10, 2019

SiTaggart commented Oct 10, 2019

TheSisb commented Oct 10, 2019 • edited Loading

richbachman commented Oct 10, 2019

serifluous commented Oct 11, 2019

TheSisb commented Oct 15, 2019

serifluous commented Oct 16, 2019

TheSisb commented Oct 16, 2019 • edited Loading

serifluous commented Oct 16, 2019

SiTaggart commented Oct 17, 2019 • edited Loading

richbachman commented Nov 5, 2019

TheSisb commented Oct 9, 2019 •

edited

Loading

vercel bot commented Oct 9, 2019 •

edited

Loading

TheSisb commented Oct 10, 2019 •

edited

Loading

TheSisb commented Oct 16, 2019 •

edited

Loading

SiTaggart commented Oct 17, 2019 •

edited

Loading