fix(icons): draw everything from one Iconify set - #290
Closed
pathscale wants to merge 1 commit into
Closed
Conversation
A consumer installs icon sets itself, so every set this library reaches for is a set the consumer has to know to install. Nothing stated the requirement and nothing failed loudly when it was unmet: the build printed `Cannot load icon set for "mdi"` among its warnings and the icon rendered as empty space. crates.vip installed `@iconify-json/lucide`, which is what the fleet uses. Five icons were `mdi` -- in `LanguageSwitcher`, `ThemeColorPicker`, `MobileListView` and the Firefox banner -- and all five were blank. This library develops against `@iconify/json`, the whole collection, so it could never see that. Four had direct lucide equivalents and were converted. The fifth is the Firefox brand mark, and lucide has no brand glyphs, so it becomes a prop on `FirefoxPWABanner` with no default: a consumer who wants it supplies it and installs the set it needs, and everyone else pays nothing for a banner they never render. The verifier walks the source for `icon-[set--name]` and fails on any set but `lucide`, skipping comment lines so the one that names `mdi` in prose does not trip it. Reintroducing a single `mdi` icon fails it by file name.
Owner
Author
|
Rolled into #289 so the four fixes ship as one release. The commit is unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A consumer installs icon sets itself, so every set this library reaches for is a set the consumer has to know to install. Nothing stated that requirement, and nothing failed loudly when it was unmet — the build printed
Cannot load icon set for "mdi"among its warnings and the icon rendered as empty space.Where it showed up
crates.vip installs
@iconify-json/lucide, which is what the fleet uses. Five icons in this library weremdi, acrossLanguageSwitcher,ThemeColorPicker,MobileListViewand the Firefox PWA banner. All five were blank, and the only signal was five identical warnings in a build log that also prints unrelated ones.The library develops against
@iconify/json— the whole collection — so it can never see this for itself. That is why the fix comes with a test rather than a correction.The change
Four had direct lucide equivalents:
mdi--palettelucide--palettemdi--loadinglucide--loader-circlemdi--inbox-outlinelucide--inboxmdi--closelucide--xThe fifth is the Firefox brand mark, and lucide has no brand glyphs. Rather than oblige every consumer to install all of
@iconify-json/mdifor one banner in one optional component, it becomesFirefoxPWABanner.iconwith no default: supply it and pay for the set, or omit it and the banner renders without a mark. It accepts whatIconaccepts, so an inline SVG works too.The verifier
tests/components/single-icon-set.test.tswalks the source foricon-[set--name]and fails on any set butlucide. It skips comment lines, so the doc comment that namesmdiin prose does not trip it, and it asserts a floor on the number of references it found so a broken walk cannot pass silently — that guard immediately caught my own wrong count.Reintroducing one
mdiicon fails it by file name:Verification
bun run testmdiicon restoredbunx tsc --noEmitbun run check:apiRelease
Third fix wanted in 3.1.1, with #288 and #289. All three are independent.