Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from Nov 5, 2019

Conversation

@TheSisb
Copy link
Collaborator

TheSisb commented Oct 9, 2019

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.

@now

This comment has been minimized.

Copy link

now bot commented Oct 9, 2019

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

This comment has been minimized.

Copy link
Collaborator

SiTaggart commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

SiTaggart commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

TheSisb commented Oct 10, 2019

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:
image

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

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

This comment has been minimized.

Copy link
Collaborator

richbachman commented Oct 10, 2019

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:

image

@serifluous

This comment has been minimized.

Copy link
Contributor

serifluous commented Oct 11, 2019

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:

image

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 TheSisb force-pushed the icons/migrate-to-streamline branch from ba83e8c to 5f4e98d Oct 15, 2019
@TheSisb

This comment has been minimized.

Copy link
Collaborator Author

TheSisb commented Oct 15, 2019

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

@serifluous

This comment has been minimized.

Copy link
Contributor

serifluous commented Oct 16, 2019

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

@TheSisb

This comment has been minimized.

Copy link
Collaborator Author

TheSisb commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor

serifluous commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

SiTaggart commented Oct 17, 2019

super crispy 👌

@TheSisb TheSisb force-pushed the icons/migrate-to-streamline branch from d920c3e to bc746d6 Nov 4, 2019
@richbachman

This comment has been minimized.

Copy link
Collaborator

richbachman commented Nov 5, 2019

This looks good to me. Nice job!

@TheSisb TheSisb merged commit 571791d into master Nov 5, 2019
7 checks passed
7 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: applitools Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
tests/applitools No visual tests ran, see "Details" for help
Details
@TheSisb TheSisb deleted the icons/migrate-to-streamline branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.