Octicons v2

[emplums]

This PR updates Primer React Components to use @primer/styled-octicons instead of @primer/octicons-react

🚨 Breaking Changes 🚨

• Primer React Components now requires users to install @primer/octicons-react
• StyledOcticon has been deprecated

TODO

• Create the @primer/styled-octicons package to replace StyledOcticon Create @primer/styled-octicons package octicons#417
• Make @primer/styled-octicons a peer dependency
• Write docs for using @primer/styled-octicons in Primer Components
• Update types
• Fix tests

Notes

• The CircleOcticon component looks buggy because the size of the icon and the size of the circle are set to the same value. This isn't new in this PR, it's always been visually broken like this but seems more noticeable with some of the newer Octicons. I created a follow-up issue here

[vercel]

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

🔍 Inspect: https://vercel.com/primer/primer-components/2a5esddzx
✅ Preview: https://primer-components-git-octicons-v2.primer.vercel.app

[emplums]

It doesn't look like @primer/styled-octicons exports types so I think we're going to need to do that over in that repo before merging this PR

[emplums]

Just kidding, looks like it does: https://github.com/primer/octicons/blob/master/lib/octicons_styled/script/build.js#L30

[emplums]

Game plan as of last week:

• Add deprecation notice for requiring a theme provider
• Keep @primer/octicons-react as a peer dependency

Open Questions:

• Should we keep StyledOcticon and not use @primer/styled-octicons afterall?

@BinaryMuse brought up a good point that it feels a little weird to have a library that implements primer-y react components separate from @primer/components but I'm still not sure - @primer/styled-octicons doesn't really implement any primer-specific styles, it just gives access to styled-system and uses whatever theme you pass it to create styles. Technically it could be used with any design systems set of spacing/color/etc values. I view it more as a "tool" than something specific to Primer React Components. In concept it feels closer to octicons than Primer React Components which is why I think it makes sense living in the octicons monorepo. 🤔

Ultimately I don't think it matters too much where it lives. Whether or not we use @primer/styled-octicons or keep a StlyedOcticon component with @primer/octicons-react doesn't change whether or not we'd need to list the icon library as a peer dep. One benefit of keeping StyledOcticon is that it would maintain some of the current API of interacting with icons in Primer React Components. We also wouldn't need to deal with the issue of requiring a ThemeProvider since we could set the default theme in the StyledOcticon component

[emplums]

This week I've been testing out whether or not using something like StyledOcticon with the new icon components in @primer/octicons-react will work - I remember Cole mentioning something about how the new API of the components (on base component vs the old wrapper around a base component) might make forwarding styles down to the appropriate DOM elements tricky, but I'm testing that out this week to see if I can come up with something that works!

[emplums]

Ok! After much discussion with @BinaryMuse we've decided to keep StyledOcticon around for now, in lieu of using @primer/styled-octicons. I know we previously discussed this back in April but now with a clearer idea of the pros and cons involved, I think the best choice for us going forward is to use StyledOcticon + @primer/octicons-react instead of using @primer/styled-octicon.

Here's the reasoning:

• Using @primer/styled-octicons would mean that we'd expect users to always wrap their application in a ThemeProvider with the @primer/components default theme passed in. In general, that's not a bad idea anyways, but right now that would be quite a breaking change for folks. Users of Primer React Components expect all components & access to the theme to work right out of the box so it feels jarring for that not to be the case with @primer/styled-octicons
• As previously discussed icon components wrapped with styled system and our COMMON and sx prop linked to our theme are more of an extension of Primer React Components than of Octicons, so it makes sense for them to live inside of Primer React Components
• Although it's been mentioned that using StyledOcticon feels a bit clunky, using @primer/styled-octicons instead would mean that we'd be adding another peer dependency, and we'd also require a ThemeProvider with access to the default theme which feels clunky as well
• If we do change our mind and decide to use @primer/styled-octicons afterall, it will be much easier to make that change than it will be to undo things the other way around

I'm still open to deciding to use @primer/styled-octicons in the future and would be curious to hear what Cole's thoughts are when he's back, but going forward with this approach allows us to get the new icons in ASAP (which are quite overdue 😬 ) and make the decision about whether or not to use @primer/styled-octicons at a later time. I imagine that @BinaryMuse 's work on theming over the next month should reveal some better guidelines around theming that might allow us to make a more informed decision anyways!

[emplums]

I'm going to close this PR and open a new one that's not a hot mess, since changes for the approach going forward should be pretty minimal 😂