RFC: remove icons from themes

[layershifter]

Issue? 💣

Icons in theme create bloat as they are not tree-shakable, see #2113. Currently we have 178 icons that will be imported for Teams theme always. They are also defined as React JSX elements so they are not framework agnostic and can't be reused without React.

Story ⌛️

We placed icons under the theme with an assumption that they are part of branding and it's true. If icons are the part of themes it allows to power theme switching capabilities and use the same code with different branding.

But it's a big lie as it requires to align names for everything, i.e. arrow-right and arrowRight are different things and it will break theme switching. I think that our requirement with having icons under theme is broken, but can but there are some cases with embedded higher order components, see below.

Proposal ✋

Do not use Icon component internally

• it creates a hidden dependency on icon-menu-arrow-end
• it creates a dependency on Icon component

In Semantic UI in such cases styles are defined in component:

My proposal is to embed these icons to component's styles:

Icon.create(submenuIndicator, {
  defaultProps: () => ({
    name: 'icon-menu-arrow-end',
    styles: styles.submenuIndicator,
  }),
})
// becomes
Box.create(submenuIndicator, {
  defaultProps: () => ({
    styles: styles.submenuIndicator,
  }),
})

Solve Icon component

I propose to remove icons from themes at all and then I see three options how icons can be used.

<BookIcon size='large' />

Use icons as separate React components like in react-icons & grommet-icons.

<BookIcon />
<CameraIcon />

• 👍 obvious dependency
• 👍 each icon can have specific props
• 👍 type safe
• 👎 ???

<Icon name="book" />

Register icons manually before usage like in react-fontawesome.

icons.register(bookIcon)
//
<Icon name="book" />

• 👍 keeps existing API
• 👎 not type safe
• 👎 not safe:
  • if an icon will be removed it should be removed in two places
  • easily to forgot to do registration

<Icon definition={bookIcon} />

Use an object with definitions instead of name like in react-fontawesome.

<Icon definition={bookIcon} />
<Icon definition={cameraIcon} />

• 👍 obvious dependency
• 👍 type safe
• 👍 close to the existing API
• 👎 custom props can be an issue

Embedded components 🏭

We still need to have a recipe for embedded components, I propose two options.

Option 1: React.Context or props approach

Icons be mapped to props via Provider component and then can be used inside of it.

Demo: https://codesandbox.io/s/mystifying-tree-qpcq3

• 👍 type safe
• 👍 good bundle size (only used icons will be really included)
• 👎 explicit configuration, can be solved with defaults options (will increase bundle size)

Option 2: CSS approach

Even SVGs can be used as url() (converter).

Demo: https://codesandbox.io/s/wandering-fire-xpqlw

• 👍 no React specific things
• 👎 can increase bundle size

[mnajdova]

Thanks for the detailed RFC, I need some clarifications.

Do not use Icon component internally

Your proposal here is to embed these icons to component's styles. Does this mean that we should use svgs with url() inside the styles? Should we make the svgs functions so we can allow different colors when the themes are switched?

Solve Icon component

General question I have here for all three icons is, how should we customize the icon (style them) when different themes are applied. The question is mainly around let's see changing the colors of some parts of the svgs in dark or light theme. Should the parent style's be responsible for this? This can be a bit problematic, as the parent should know the actual structure of the svg icon used at that moment..

<BookIcon size='large' />

👎 Need to maintain big set of components

From the three approaches, I would say that I prefer the third one <Icon definition={bookIcon} /> mainly because we don't have to have separate component for each Icon. Can you clarify what exactly do you mean by the down-vote: 👎 custom props can be an issue

Embedded components 🏭

In my opinion Option 1 probably this is a better idea, mainly because of the styles that needs to be applied on the icons

[levithomason]

The data approach seems promising, though we need to prove styling works (color/stroke/fill/currentColor/etc). Also, hover and other states need to work.

[levithomason]

As pointed out, actually using data in styles will not help bundle size since all styles are included for every icon as soon as you get the icon styles.

[dzearing]

Adding my 2c:

From what I've seen, icons really should be treated as components. They may have very specific props depending on their implementation. An image icon may not be able to scale as effectively as an SVG or font icon, and may be locked to 3 sizes. An svg icon from a different library might have a color or animation related prop to trigger effects or styling in some cases.

So far, we have made icons injectable from an external source in v0 and v7 code. In v0 code, icons are provided through the theme. In v7, there are icon apis to set/get them globally (registerIcons, getIcon). Both approaches suffer from what you're calling out:

• No type safety, easy to break
• No idea how to control more specific icon props from the component usage
• Requires either huge bundle or tedious micromanagement from the consumer

If we stop and think of icons as components as you've called out in the first example above: <AddIcon /> from 'react-icon-set'... Then from a consumer's perspective we've given them type safety and we've trimmed the bundle to exactly what needs to be included.

However; "how do we make icons theme-able by the app" is not solved. The question now becomes "how do we make components replacable through context?"

What if we approached that problem in a broader sense? For example:

// Use some utility which wraps the component and checks with context for replacements before rendering.
export const AddIcon = Replacable(AddIcon);

// Use it with specific props:
<Button icon={ (buttonState) => <AddIcon isAnimating={buttonState.loading} /> } />

// Wrap the app with a "component replacer" solution that is type-safe:
<ReplacementsProvider overrides={{
  components: {
    ...AddIcon.replace(
      props => props.isAnimating ? <Animated><NewAddIcon/><Animated> : <NewAddIcon ... />
    )
  }
}

Something like that might work for cases where we need to replace a component with another, and at least we'd get some amount of type safety with input props.

Downsides:

• Downloading "default" component AND "replacement" component
  others?

[dzearing]

I thought of one other downside here; react context based solutions would create a hard requirement on react. The css approach would be easier for framework agnostic icon replacement but still suffers from the other problems.

Even with css only icons, hard to visualize how replacements work well. They are version and name sensitive without good type safety.