Dynamically import icon svgs instead of building them into bundles#1822
Dynamically import icon svgs instead of building them into bundles#1822chandlerprall wants to merge 15 commits intoelastic:masterfrom
Conversation
|
@chandlerprall So the problem I'm running into is that I can't transition between the loading state with So we could create a different div that sits beneath the SVG, but then we have this extra lingering div and have to deal with absolute positioning. Can you think of another option? |
|
@cchaos I can add a small delay to removing the loading css class, which should allow any css transitions to apply. Want to try that? |
|
My only concern with that is that we're then forcing a minimum time period of no-icon even if it's not necessary. |
|
PR4U: chandlerprall#8 Here are all the gifs of the different sections working so others can take a glance. I slowed my network down to "Fast 3G" in dev tools. Glyphs Example of usage with EuiNavDrawer |
|
added @pugnascotia as a reviewer on the cloud consuming side of things |
|
@chandlerprall I pushed a fix for FF |
|
IE seems to be fine it almost waits for all the icons before it renders the page, so I can't see the |
|
You'll need to rebase with master and update a few icons that were added in the meantime |
…i into feature/icon-breakapart
|
Merged with changes on master (4 new icons, some typescript clean ups), and cleaned up loading an external image source. |
The |
Nope! Babel isn't configured to process |
Verified. However, the addition of the dynamic import babel plugin means that running Kibana locally with a |
|
I had to do the following to the source (and then build) to get it to work with Cloud UI: diff --git a/src/components/icon/icon.tsx b/src/components/icon/icon.tsx
index 2b8b21f4..4d456b68 100644
--- a/src/components/icon/icon.tsx
+++ b/src/components/icon/icon.tsx
@@ -396,7 +396,8 @@ export class EuiIcon extends Component<Props, State> {
if (isIconType(this.props.type)) {
isLoading = true;
- import(`./assets/${typeToPathMap[this.props.type]}.js`).then(
+ // tslint:disable-next-line
+ import('./assets/' + typeToPathMap[this.props.type] + '.js').then(
({ icon }) => {
this.setState({
icon,It's to do with how Babel transpiles template strings and Webpack contexts. With the above, icons are loaded from their own webpack chunks. |
snide
left a comment
There was a problem hiding this comment.
I did a quick check against the functionality we need. Custom images work the way I expect. Before I go into a deeper review, we definitely will need to add the following in here:
- An example showing custom svg use.
- Prop docs explanations.
- Some snippets would likely help.
- Docs to explain how this work as far as theming. Specifically thinking we'll need to document the class names we use for fills.
- Likely some rewrite of our instructions on the top of the page now that this stuff is all different.
If you'd like, I can do a PR for you to handle this. Might have some questions.
@snide I'd appreciate it a lot :) |
|
closing in favor of creating a larger feature branch & PR against that |
Summary
Changes EuiIcon to dynamically load SVGs. This has been tested in Kibana and confirmed to work with the webpack setup there. EuiIcon's tests themselves still snapshot the full, rendered, SVG contents. Other snapshots capture the loading state (as will be the case with downstream tests in Kibana, this should not affect Cloud UI as they already mock EuiIcon).
Note this requires manually running the
yarn compile-iconscommand for any new icon or modifications to an existing one, replacing the manual clearing of jest cache.Caroline is aware of a bug in FF around the loading animation
Checklist
- [ ] Any props added have proper autodocs- [ ] This was checked against keyboard-only and screenreader scenarios- [ ] This required updates to Framer X components