Use StrataKit icons - #1587
Use StrataKit icons#1587GerardasB wants to merge 44 commits into
StrataKit icons#1587Conversation
| /** Dynamically imports a module. Useful for optional dependencies that may not be installed in the project. | ||
| * @internal | ||
| */ | ||
| export function useOptionalModule<T>(importFunc?: () => Promise<T>) { |
There was a problem hiding this comment.
@saskliutas, @iTwin/stratakit-developers Currently I am using dynamic imports to load the optional StrataKit modules (@stratakit/foundations and individual StrataKit icons). This adds some complexity and async behavior to the code e.g. the icons only start loading once rendered. See:
This was done to facilitate consumers that do no have StrataKit installed: using static imports would cause bundlers to fail when StrataKit is not installed (even if the code is never executed, unless the useStrataKit preview feature is enabled).
While this is the "correct" way to avoid having to release a new major version of AppUI, maybe it would be fine to either:
- Define
@stratakit/iconsand@stratakit/foundationsas non-optional peer dependencies (i.e. all consumers updating to a new minor version of AppUI would need to install these packages, however they would not be "used" unless theuseStrataKitpreview feature is enabled) - Define
@stratakit/iconsand@stratakit/foundationsas dependencies (i.e. all consumers updating to a new minor version of AppUI would automatically get these packages installed). This might cause issues with multiple versions of StrataKit being installed in the same project.
There was a problem hiding this comment.
I'm not sure that this will work in practice. Dynamic imports still get processed by build tools so that they can output the correct files (ref: Vite docs, Rsbuild docs). If the package is not installed, this could lead to a build error, making this no different from static imports. It's worth testing this claim in a standalone starter app to be sure.
@stratakit/foundations should not be a direct dependency. It will throw an error if multiple versions are detected. We've been asked to make it a peer dep even in our own packages (@stratakit/mui, @stratakit/structures).
There was a problem hiding this comment.
The peerDependency approach sounds better. Then the consumer's bundler can decide when and how to bundle the icons.
There was a problem hiding this comment.
I'm not sure that this will work in practice.
Tried it in an empty vite project. Seems like the dev server doesn't like the dynamic import of SVG assets (import of @stratakit/foundations is working as expected): [plugin:vite:import-analysis] Failed to resolve import "__vite-optional-peer-dep:@stratakit/icons/placeholder.svg:@itwin/appui-react" from "node_modules/.vite/deps/@itwin_appui-react_tmp.js?v=6eca5e37". Does the file exist?. When built pnpm build -> pnpm preview seems to work fine for icons as well. Vite config modifications are no better than having to install the package.
Ideally, if the StrataKit imports would be done from a separate subpath that would solve all the issues (and the need for dynamic imports), not sure how feasible is that given the requirement of doing it once at an application level (not in each AppUI import). EDIT: e.g.: WIP: useStrataKit subpath export
For now, if there are no objections, to avoid complexity - let's just go with static imports and assume that consumers will install the required "optional" StrataKit dependencies.
There was a problem hiding this comment.
This seems good as the general approach (assuming it works with vite and rsbuild projects). I like that it hides all the complexity from the consumer.
My main feedback would be to preserve the entire import path at the call site for easier migration in the future. For example:
const { Icon } = modules?.["@stratakit/mui"];
const svgDockLeft = modules?.["@stratakit/icons/dock-left.svg"];1:1 equivalent to:
import { Icon } from "@stratakit/mui";
import svgDockLeft from "@stratakit/icons/dock-left.svg";a8eea6a to
c1442c8
Compare
7e4b4cc to
abf2fdb
Compare
50675f1 to
443865a
Compare
443865a to
8bf5909
Compare
d43d8da to
a03fe85
Compare
| {isLocked && <StrataKitIcon href={svgLock} iconNode={<SvgLock />} />} | ||
| {isLocked && ( | ||
| <StrataKitIcon | ||
| module="@stratakit/icons/lock.svg" |
There was a problem hiding this comment.
Looks like the module prop added in 089997a disconnects the imports from the specifiers. In #1587 (comment), I was suggesting keeping the two close to each other to make future changes easier.
If you wanted to use the new named icon exports (e.g. svgLock & svgLockLarge), that wouldn't possible with this approach, right?
Changes
This PR adds
useStrataKitpreview feature, which when enabled renders StrataKit icons instead of iTwinUI icons. This can only be used after enabling the StrataKit theme bridge.To enable the preview feature, a separate subpath is exposed to make sure that imports are not considered when the StrataKit packages are not installed, see #1587 (comment)
Notable changes:
useStrataKitpreview feature@bentley/icons-generic-webfontare replaced with StrataKit icons using an internal mappingTheme bridgetoolbar item can be used to enableuseStrataKitpreview feature (this will need additional work, since different decorators are used to setup different stories)@itwin/appui-react. TheStrataKitIconcomponent is injected via context to lower level packages viaStrataKitIconContextof@itwin/core-reactUsed scripts
Script to update webFontToStrataKitIcon.ts
Extract imports for useStrataKit.ts
Screenshots
Testing
Tested in
test-appby enabling the theme bridge and preview feature: http://localhost:3000/blank?frontstageId=main&themeBridge=1Tested in storybook by enabling the
themeBridge:useStrataKitglobaluseStrataKit: https://itwin.github.io/appui/1587/?path=/story/components-solartimeline--basicuseStrataKit: https://itwin.github.io/appui/1587/?path=/story/components-solartimeline--basic&globals=themeBridge:useStrataKit