feat(icon)!: icon 1:1

[bennypowers]

Let it be known ahead of time: there will be rebasings

Demo

:zap: :mage: Behold! :eye: :crystal_ball:

Notes for Reviewers

Dependent Components

• pfe-accordion (don't mind changing up the icon here, but I'd rather avoid bikeshedding b/c anyways we're 1:1-ing the accordion element)
• pfe-button
• pfe-clipboard
• pfe-label
• pfe-modal
• pfe-select

Accessibility Considerations

The shadow container for icon will be removed from the a11y tree if the label attribute is unset.

Ready-for-merge Checklist

• [x] Tests have been updated to cover these changes.
• [x] Changesets added.
• [x] Documentation (README.md, docs pages) updated or added.
• [x] Dependent elements updated
• [x] commitlint errors can be safely ignored this time

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 7256c7b4ee33ddded6ff3a8cb94a14d618f9bcb9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions[bot]]

👕 Commitlint Problems for this PR:

🔎 found 2 errors, 0 warnings ℹ️ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

97514314 - docs(cta): update icon usage

• ❌ scope must be one of [ci, config, create, dependencies, deps, docs, scripts, accordion, autocomplete, avatar, badge, band, button, card, clipboard, codeblock, collapse, datetime, dropdown, icon, jump-links, label, modal, page-status, progress-steps, readtime, select, spinner, tabs, tooltip, core, sass, styles, create-element, eslint-config, netlify-plugin-github-actions, tools]

0fdc07a4 - fix(icon-panel)!: delete

• ❌ scope must be one of [ci, config, create, dependencies, deps, docs, scripts, accordion, autocomplete, avatar, badge, band, button, card, clipboard, codeblock, collapse, datetime, dropdown, icon, jump-links, label, modal, page-status, progress-steps, readtime, select, spinner, tabs, tooltip, core, sass, styles, create-element, eslint-config, netlify-plugin-github-actions, tools]

[github-actions[bot]]

✅ Deploy Preview for patternfly-elements ready!

Name	Link
🔨 Latest commit	d02a558ec5077d8b04bede47dbad8ccfef97a8c9
😎 Deploy Preview	https://deploy-preview-2138--patternfly-elements.netlify.app/

---

To edit notification comments on pull requests, go to your Netlify site settings.

[heyMP]

@bennypowers, @zeroedin and I are seeing this error on node -v 16.

   npm run start

> start
> npm-run-all build:tools build:icons build:analyze --parallel dev:* watch:docs:analyze


> build:tools
> npm run build --workspace @patternfly/pfe-tools


> @patternfly/[email protected] build
> tsc -b .


> build:icons
> ts-node --esm --project tsconfig.icons.json ./scripts/icons/build.ts

SyntaxError: Unexpected identifier
    at Loader.moduleStrategy (node:internal/modules/esm/translators:146:18)
ERROR: "build:icons" exited with 1.

We tried removing package-lock.json, removing node_modules, pinning esbuild to 0.15.5. The only solution that worked for me was changing to node -v 18.

[bennypowers]

hmm

$ node -v
v16.15.1
$ npm run build:icons

> build:icons
> ts-node --esm --project tsconfig.icons.json ./scripts/icons/build.ts

(node:65545) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
$

[bennypowers]

do you have a global tsnode? if i had to guess, i'd say it's probably something in the toolchain choking on the import assertions in that file

[zeroedin]

global tsnode

I ran npm list -g --depth 0 to look at my global packages

I do not have a global tsnode. The only global packages I have installed in the nvm version are: node and yarn.

[bennypowers]

other thing it might be is linux versions of esbuild in the package lock vs mac versions. i'll try running it on macos and see

[bennypowers]

well, I did a couple of things:

cd patternfly-elements
git stash
git stash
git checkout main
git checkout feat/pf-icon-1-1
git pull
npm run clean:nuke
npm ci

# install missing esbuild binaries, and write them to package lock
npm i -w tools/pfe-tools esbuild --include=optional
git add tools/pfe-tools/package.json package-lock.json
git commit -m "chore: include optional esbuild deps"
git push

With those done, I was able to run the dev server both on linux and on macos

git pull
npm run clean:nuke
npm ci
npm start

Try running that last snippet

[heyMP]

@bennypowers

Try running that last snippet

I ran those steps and unfortunitely it didn't fix the issue. Running on node v16.11.0

Is it possible you are on a version greater than version 16?

[bennypowers]

16.15.1

[heyMP]

Wow, updating to 16.15.1 fixed it for me 😂

[bennypowers]

thanks for the help debugging. bumped the nvmrc

[brianferry]

@bennypowers - Seeing an issue on the docs page on the demo site where the icons don't appear to be showing on the main docs page but they do show on the /demo page. Thinking it might be the relative paths of the icons?

[bennypowers]

alright give that another look

[bennypowers]

@heyMP @zeroedin @brianferry thoughts on the non-bubbling error event?

alternatives:

• bubble it but require users to handle it before it throws when it reaches the window
• rename it icon-error load-error fail etc
• keep it like this, but make users who need to catch them for analytics fire their own events

[brianferry]

@bennypowers this is great, love a lot of the upgrades & updates that have been made in the PR. Especially like how clean / concise the icons are now. Left a few question / minor notes in here but I think these changes are great

[mwcz]

I finally took a look at this today. Thanks @bennypowers for the reminder.

It looks like the two main high level changes are the SVG loading mechanism (raw SVG -> ESM), and the SVG injection mechanism (SVG <image> tag -> embed in shadow DOM). It all looks good to me so consider this a :+1:. Here's some old thoughts from the dawn of pfe-icon, in case they're interesting or useful.

Loading mechanism

Old pfe-icon loads raw SVGs in order to have a very low barrier when adding new or custom icons. I use that pretty frequently, so needing to embed the SVGs each in an ES module adds some friction. I spoke with @bennypowers about this and the idea of splitting the converter out into its own package seemed like a good one (TBD after this PR). I'm picturing a svg2esm package that could be run easily on the command line or imported as a library.

Injection mechanism

The ability to render the icon as an "in-tree" SVG so to speak, rather than in an SVG <image> element, certainly makes coloring the icon easier.

Though, loading them in an <image> has some benefits and tradeoffs as well:

1. <image> protects the page from scripts embedded in SVGs
2. <image> prevents pfe-icon's shadow DOM CSS from applying to the SVG's inner elements, and prevents CSS embedded in the SVG to leak out into the shadow DOM (pro and con! as this also makes it harder to style/colorize the icon, hence why the old method used filter+feFlood)
3. <image> is handled by the browser similarly to HTML's <img> in that the same-origin policy is relaxed. Including an SVG with fetch or <use> (or importing an ES module) requires CORS headers for cross-origin imports. Not hard to add, but a frequent stumbling block.

[bennypowers]

interesting point about cors. I think shadow root provides the right level of encapsulation here. if svg has scripts or styles that interfere with the limited dom in the shadow root, that's a smell. you provide unminified, weird svg - that's on you.

as for <script>, i'm pretty sure that won't run anyways

what does the cli for the hypothetical converter look like?

svg2esm ./icons/**/*.svg -o ./build/icons/

[brianferry]

Looks at the code everything looks good, was able to build & serve locally. I'll do a follow up for the pfe-accordion icon just so we have it in there.