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

Feature: Allow dynamic import of icon for easier bundling #2025

Merged
merged 13 commits into from
Jun 24, 2024

Conversation

madsrasmussen
Copy link
Contributor

@madsrasmussen madsrasmussen commented Jun 21, 2024

Description

Currently, we import icons based on a relative path. This causes some trouble when bundling the icon-registry module as files might not be located in the same place after bundling. This PR adds the extra option to "manually" import the icon file (the same way we do in manifests) like path: () => import('./path/to/icon.js'). This way the bundler knows where the files are located and will handle them correctly.

What to test

  • Make sure that icons render correctly when running with Vite
  • Make sure that icons render correctly when building for the CMS.
  • All icons are still lazy loaded when they appear on the screen.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Copy link
Collaborator

@iOvergaard iOvergaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the premise of this. Can I ask what the need is to change all icons to .ts files?

@madsrasmussen
Copy link
Contributor Author

@iOvergaard The only reason was that I started to get type errors with js files. I don't know how we could avoid that before. 🤔

Screenshot 2024-06-24 at 10 31 25

@iOvergaard
Copy link
Collaborator

@iOvergaard The only reason was that I started to get type errors with js files. I don't know how we could avoid that before. 🤔

Screenshot 2024-06-24 at 10 31 25

I think it worked because it was just a variable, and we additionally had Vite ignore it on the dev server with:

import(/* @vite-ignore */ iconPath)

@iOvergaard iOvergaard merged commit df4f3f6 into main Jun 24, 2024
6 checks passed
@iOvergaard iOvergaard deleted the v14/feature/allow-dynamic-import-of-icon branch June 24, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants