An error occurred while fetching the assigned milestone of the selected merge_request.
feat: dynamic icon component
1 unresolved thread
1 unresolved thread
Merge request reports
Activity
Filter activity
requested review from @moritz
assigned to @truongleit
@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.
added 1 commit
- c68058de - fix(icon-component): export and add color prop
I have some problems with this approach:
- It always uses Carbon icons. It should still be possible to pass other Icon components, if someone doesn't want to use Carbon icons.
- 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.
I agree with the suggestion to move this component to Astro repo. For other points, I have some concerns:
- 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?
- 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.
- 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