Skip to content
Snippets Groups Projects

feat: dynamic icon component

Closed Truong Le requested to merge feat/icon-component into main
1 unresolved thread

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Truong Le requested review from @moritz

    requested review from @moritz

  • assigned to @truongleit

  • Author Contributor

    @moritz Can you have an initial look at the current work? I believe we need an agreement on the approach before going further by adding this to other components.

  • Truong Le added 1 commit

    added 1 commit

    Compare with previous version

  • Truong Le added 1 commit

    added 1 commit

    • a42784c9 - fix(icon-component): fix size property

    Compare with previous version

  • Truong Le added 1 commit

    added 1 commit

    Compare with previous version

  • Truong Le added 1 commit

    added 1 commit

    • c68058de - fix(icon-component): export and add color prop

    Compare with previous version

  • I have some problems with this approach:

    1. It always uses Carbon icons. It should still be possible to pass other Icon components, if someone doesn't want to use Carbon icons.
    2. It bundles the whole Carbon icon set into our UI library, which bloats the package a lot and might even be problematic for licensing reasons (not sure about that).

    I'd rather handle this inside the Astro repo, and not inside the UI library. The UI library is supposed to stay "dumb"/generic.

    • Author Contributor

      I agree with the suggestion to move this component to Astro repo. For other points, I have some concerns:

      1. Even if using dynamic import, we still need to install the icon packages first. So all the icon packages that content writers will use should be installed before they can use it inside MDX. E.g:
      import('react-simple-maps').then(module => module)
      • The reason I imported the whole package into the project is that icon package uses named export for all the icons. So if we do something like every time we use the icon component, that is a huge waste (importing whole big object and only take one property out:
      import('icon-package').then(module => module[iconName])

      Any ideas?

      1. Yes, of course we need to install the icon package – there's no way around it. But including it in the UI package will increase the package size a lot (which doesn't really matter in our Astro project, but might matter to other projects which might want to reuse the UI library). Let's try to keep the UI library small.
      2. We can still use this import approach in the Astro repo, because we don't need to care about package/bundle size, when we generate it on the server.

      So I guess to sum it up: Just move this over to the Astro repo and create wrapper components for all UI components that have a icon prop.

    • Please register or sign in to reply
  • closed

Please register or sign in to reply
Loading