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

Why not use a single component with content or prop as icon? #8

Open
Devmond opened this issue Jun 9, 2020 · 12 comments
Open

Why not use a single component with content or prop as icon? #8

Devmond opened this issue Jun 9, 2020 · 12 comments
Labels
question Further information is requested

Comments

@Devmond
Copy link

Devmond commented Jun 9, 2020

Instead of constructing potentially hundreds of individual components to represent each icon, why not a single component that uses either a prop or content for the icon name?

We build a low-code developer tool hence the perspective that using a single component would be a much better approach.

@ismamz
Copy link
Owner

ismamz commented Jun 9, 2020

Hi @Devmond, how would you do that? Do you have an example to show me?

@Devmond
Copy link
Author

Devmond commented Jun 9, 2020

Hi @ismamz you can checkout font awesome: https://github.com/FortAwesome/react-fontawesome

they implement it using a single icon component then use a prop to select a specific icon.

@ismamz
Copy link
Owner

ismamz commented Jun 10, 2020

This library is highly inspired by react-feather. Bootstrap has more icons and therefore this module is bigger.

The approach that you propose is super valid, but I think that there are two different paths and each one has their pros and cons.

However, I think that react-feather and this package has a simpler API. In your final bundle you will only include the icons you use and they are simple React components (SVG) with no dependencies.

I'm open to listen to any proposal of how to even mix these two approaches in the same library.

@rasgo-cc
Copy link

rasgo-cc commented Nov 5, 2020

I'm with @Devmond on this one. I also prefer react-fontawesome approach because it allows me to abstract the icons components more easily. Like this:

const Icon = (props: Props) => {
  const { icon, className: iconClasses, ...otherProps } = props;
  return (
    <FontAwesomeIcon
      {...(otherProps as any)}
      icon={icon as any}
      className={iconClasses}
      fixedWidth
    />
  );

In this case I'm only rendering fontawesome icons but I could easily pass a bootstrapIcon prop to tell to render a bootstrap icon instead, where I'd have a <BootstrapIcon /> component responsible for the rendering.

@tkrotoff
Copy link

tkrotoff commented Nov 6, 2020

@rasgo-cc

because it allows me to abstract the icons components

No problem to do that with react-bootstrap-icons approach, this works perfectly:

// File Icon.tsx

import React from 'react';
import * as bootstrapIcons from 'react-bootstrap-icons';

// FIXME https://github.com/ismamz/react-bootstrap-icons/issues/14
interface BootstrapIconProps extends React.SVGAttributes<SVGElement> {
  color?: string;
  size?: string | number;
}

interface Props extends BootstrapIconProps {
  // Cannot simply use "name" as it is a valid SVG attribute
  // "iconName", "filename", "icon"... will do it instead
  iconName: keyof typeof bootstrapIcons;
}

export function Icon({ iconName, ...props }: Props) {
  const BootstrapIcon = bootstrapIcons[iconName];
  return <BootstrapIcon {...props} />;
}
import { Icon } from './Icon';
...
<Icon iconName="Stopwatch" className="align-top" />
...

@rasgo-cc
Copy link

rasgo-cc commented Nov 6, 2020

Oh nice! Thanks @tkrotoff! I take back my point on the comment above 😄

@tkrotoff
Copy link

tkrotoff commented Nov 7, 2020

btw in a similar way, here a trick when testing with Jest:

// File src/__mocks__/react-bootstrap-icons.tsx

import React from 'react';
import * as icons from 'react-bootstrap-icons';

function mockIcon({ iconName }: { iconName: string }) {
  return <span>{iconName}</span>;
}

Object.keys(icons).forEach(iconName => {
  (icons as any)[iconName] = () => mockIcon({ iconName });
});

module.exports = icons;

You will output <span>Stopwatch</span> in the DOM instead of the whole SVG. This simplifies Jest snapshots and is easy to test with react-testing-library: screen.getByText('Stopwatch').

@AntonBaukin
Copy link

Hi @ismamz. Thanks for your work! Regretfully, this project is not so useful as it might be. Some thoughts of mine...

The project has to keep up with the original Bootstrap icons. Every change from them requires update here. In a time you'll loose an interest in doing this.

It's better to create Webpack plugin that somehow generates the icons required in the project directly from Bootstrap content. How to? Lets see...

So, we want a general component, common for every icon. We pass the icon name as a property. During the build Webpack does not statically know what icons do we actually use. Bootstrap has SVG sprite image with every of 1200+ icons that weights 600k. Also, there are various fonts around, Bootstraps' WOFF2 takes 80k.

But what if we actually use 10 icons from 1200. Have we to serve huge SVG sprites or fonts? The best way is to create own SVG sprite from a list of icon names. Just add icon name to JSON file of your project. Take the same JSON file for icon name property validation. That's it.

I'm going to use svg-spritemap-webpack-plugin for this.

@ismamz
Copy link
Owner

ismamz commented Jan 25, 2021

Hi @AntonBaukin, I appreciate your feedback!

About keeping sync between this library and the original Bootstrap icons, I agree with you that we need to find some consistent solution for that.

I think that the sprite solution is a good idea, also the single component approach we will probably finish adding soon.

Let me know if you have progress on that idea 👍

@johannpayer
Copy link

johannpayer commented Apr 1, 2021

I think @Devmond made a really good suggestion. The code that @tkrotoff wrote works but is really bad for bundle size because it causes Webpack to include all of the components.

@johannpayer
Copy link

johannpayer commented Apr 1, 2021

Actually, I think that bundle size would be an issue regardless of whether there was a single component or multiple. To fix the bundle size issue, you would either have to continue using a key prop but lazy load the components or you could replace the key prop with an Icon prop that takes in a React component and uses React.createElement to create the element. I chose the second approach for my website and saw a massive decrease in the bundle size of about 117KB.

@ismamz ismamz added the question Further information is requested label Sep 27, 2022
@woodrunsdeep
Copy link

Actually, I think that bundle size would be an issue regardless of whether there was a single component or multiple. To fix the bundle size issue, you would either have to continue using a key prop but lazy load the components or you could replace the key prop with an Icon prop that takes in a React component and uses React.createElement to create the element. I chose the second approach for my website and saw a massive decrease in the bundle size of about 117KB.

Could you please share how you did it? The exact code will be much appreciated. I faced the same bundle size problem in my project and I don't know how to solve it since I'm new to web development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

7 participants