-
Notifications
You must be signed in to change notification settings - Fork 49
Draft: Carbon Icon exploration spike - Do not merge #3382
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
didoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, some high level other more specific. Happy to discuss in more details, if you want, and exchange ideas/thoughts.
|
|
||
| {{#if this.hdsCarbon.carbonModeEnabled}} | ||
| {{#if (not-eq this.carbonIcon null)}} | ||
| {{#each this.carbonIcon.content as |node|}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] is this how it's done in Carbon too? it seems a risk here, because we're not covering all the possible SVG elements (eg. line, ellipse, g, clipPath, mask etc - there's more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the vanilla code: https://github.com/carbon-design-system/carbon/blob/main/packages/icon-helpers/src/toSVG.ts
| } | ||
| }); | ||
|
|
||
| loadCarbonIconTask = task(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[thought] what if, instead of loading the icons as unstructured JS objects and then build them at runtime as SVG, we leverage the existing icons pipeline to prepare them as Ember component (eg. as .gts template-only SVG files), and then import the component itself? this would also open the doors to do the same for the Flight icons, so you would not need two system of loading/showing icons, but only one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what they do for Svelte, it seems:
https://github.com/carbon-design-system/carbon-icons-svelte/blob/master/src/index.ts
And React too:
https://github.com/carbon-design-system/carbon/blob/main/packages/icons-react/tasks/build.js
https://github.com/carbon-design-system/carbon/blob/main/packages/icon-build-helpers/src/builders/index.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly use the SVG files that are in the package: https://www.npmjs.com/package/@carbon/icons?activeTab=code
And this, I think, would be a valid format for a .gts file:
<template>
<svg>...</svg>
</template>
(to confirm/validate with the FEF team, I would imagine)
| "test:a11y-report": "pnpm build:packages && ENABLE_A11Y_AUDIT=true ENABLE_A11Y_MIDDLEWARE_REPORTER=true ember test -f=\"Acceptance\"" | ||
| }, | ||
| "devDependencies": { | ||
| "@carbon/icons": "^11.70.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] this should be declared in the package/components/package.json file, no?
| } | ||
| } | ||
|
|
||
| get name(): string | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[thought] if we change the name, we will break tests when the "carbon" theme is turned on, and replacing the name across all the tests would be quite an amount of work
| {{else}} | ||
| <use href="#flight-{{@name}}-{{this.size}}"></use> | ||
| {{/if}} | ||
| {{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[issue] what happens here, and in the controller, when an icon is invoked with a name that doesn't have a mapping (eg. try to use lock-filled)
| return size === HdsIconSizeValues.Sixteen | ||
| ? await import('@carbon/icons/es/accessibility--alt/16') | ||
| : await import('@carbon/icons/es/accessibility--alt/24'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] would something like this work?
| return size === HdsIconSizeValues.Sixteen | |
| ? await import('@carbon/icons/es/accessibility--alt/16') | |
| : await import('@carbon/icons/es/accessibility--alt/24'); | |
| return await import(`@carbon/icons/es/accessibility--alt/${size}`) |
Usually this doesn't work for tree shakings, but in this case (as we discussed) is not really a tree-shaking done at build time, more of a dynamic import at run time of tiny bundled code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not an issue with tree shaking as much as it is with dynamic paths in general. The import just won't work unless the path is static and the build system knows about it ahead of time.
1adc14b to
cedd174
Compare
📌 Summary
This PR introduces the ability to ingest and render icons from the
@carbon/iconslibrary within the HDS. It implements a mapping layer to translate existing HDS icon names to their Carbon equivalents and updates theHdsIconcomponent to dynamically load and render these assets when the temporaryhdsCarbonservice is enabled.🛠️ Detailed description
Icon Mapping & Ingestion
We have implemented a utility to map HDS icon names (e.g.,
alert-diamond) to their corresponding Carbon icon names (e.g.,warning--diamond). This mapping is handled via a configuration array where each object defines:HdsIconComponent UpdatesThe
HdsIconcomponent has been refactored to support a dual-mode rendering strategy based on the state of thehdsCarbonservice:Standard Mode
Retains existing behavior, rendering HDS icons via the
<use href="...">SVG tag.Carbon Mode (
hdsCarbon.carbonModeEnabled):Dynamic Loading: Uses an
ember-concurrencytask (loadCarbonIconTask) triggered by a modifier to asynchronously import the specific Carbon icon definition.SVG Construction: Instead of referencing an ID, the component explicitly iterates over the Carbon icon definition (
this.carbonIcon.content) to render the raw SVG nodes (path,circle,rect, etc.) into the DOM.Attribute Handling: Automatically adjusts
viewBox,width,height, and CSS classes to align with Carbon's specifications.Note: The
hdsCarbonservice is currently implemented as a temporary mechanism to facilitate demos and feature-flagging for this integration.🎊 : Demo
https://hds-showcase-git-icon-exploration-hashicorp.vercel.app/carbon/icon
📸 Screenshots
🔗 External links
Jira ticket: HDS-5198
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.