Add unified icon system: <ol-icon>, $:icon() macro, build-time sprite - #12955
Draft
lokesh wants to merge 9 commits into
Draft
Add unified icon system: <ol-icon>, $:icon() macro, build-time sprite#12955lokesh wants to merge 9 commits into
lokesh wants to merge 9 commits into
Conversation
One curated set on a 24x24 / 2px / currentColor grid, replacing the ad-hoc
mix of PNG sprites, file SVGs, and inline-SVG-in-JS.
Source of truth: static/icons/src/{lucide,custom}/. scripts/build_icon_sprite.mjs
(make icons) emits three outputs from it:
- static/build/icons/sprite.svg - <symbol> sheet for the document/light DOM
- static/icons/manifest.json - committed icon-name list (gallery + lint)
- openlibrary/components/lit/icons.generated.js - committed Lit svg fragments
for shadow-DOM components (which can't reach the document sprite via <use>)
Delivery:
- $:icon(name, size, label) Templetor macro for server templates
- <ol-icon> Lit component (light DOM) for JS/light-DOM contexts
Both use a same-document <use href="#name">; the sprite is inlined once per page
via the icon_sprite() helper (footer), since external <use href="file.svg#id">
is unreliable on the Safari/iOS versions in our browserslist.
Shadow-DOM components inline glyphs imported from icons.generated.js instead;
ol-dialog's close icon is migrated as the first example.
Also: icon-size tokens (sm/md/lg), shared ol-icon.css, an icon gallery with
copy-to-clipboard at /developers/design#icons, and a pre-commit/npm check that
the generated sprite/manifest/module stay in sync with the sources.
Replace inline SVG glyphs with imports from the generated icon module: - OLMarkdownEditor: all 19 toolbar glyphs (wrapped in the editor's <svg> shell so toolbar CSS still governs size/stroke-width) - Close (X) icon deduped across OLChip, OlBanner, OlToast, and SearchModal to the canonical `x` glyph (removes four hand-written copies) Geometry is unchanged (the icon set was harvested from these components), so the rendered output is identical. Verified in-browser: editor toolbar, dialog/chip/ banner/toast/search-modal close icons all render as real SVG elements.
Size the ol-icon host element from its size attribute in render-blocking CSS (a plain `ol-icon` selector, so it applies before and after the component upgrades). The box is held from first paint, so no surrounding content reflows when the Lit bundle runs. The glyph still appears on upgrade — server-rendered templates that need instant, JS-free icons should use the $:icon() macro.
…gn page Add a 'which technique to use' comparison table, usage examples for all three paths, and a 'what you control' section spelling out sizing (sm/md/lg tokens + numeric escape hatch) and color (currentColor — set color on any ancestor).
A const declaration sat between the JSDoc block and the class, so the analyzer attached the @prop descriptions to the const instead of OlIcon — leaving the design-page API table empty. Move it above the doc comment so the table renders name/size/label with their descriptions.
…ns anywhere The icons-generated pre-commit hook ran the generator, which imported svgo — a devDependency absent in CI/containers/fresh checkouts, so the hook failed with ERR_MODULE_NOT_FOUND. The source SVGs are authored clean, so svgo was only merging paths; the generator already keeps just the attributes it needs. Drop svgo from the generator (pure Node built-ins now) and remove the unused sprite SVGO config. Glyph geometry is unchanged (paths are simply no longer merged).
…n-system # Conflicts: # static/css/page-design.css
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| placeholder="Filter $len(icons) icons…" | ||
| aria-label="Filter icons" | ||
| > | ||
| <ul class="icon-gallery" id="icon-gallery"> |
There was a problem hiding this comment.
WCAG 1.3.1: <ul> contains direct text content. Wrap in <li>.
<ul> and <ol> must only contain <li>, <script>, or <template> as direct children.
Details
Screen readers announce list structure ('list with 5 items') based on proper markup. Placing non-<li> elements directly inside <ul> or <ol> breaks this structure. Wrap content in <li> elements, or if you need wrapper divs for styling, restructure your CSS to style the <li> elements directly.
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.
Closes #
Adds a unified icon system — one curated set on a 24×24 grid, single 2px stroke, themed with
currentColor, delivered from a build-time sprite. Replaces the ad-hoc mix of legacy PNG sprites, stray file SVGs, and inline-SVG-in-JS. [feature]There are three ways to render an icon, all drawing from the same source so a glyph looks identical no matter which you use. Pick by where the icon lives:
1.
$:icon()macro — server-rendered templates. The default.$:icon("search") $:icon("globe", size="lg", label=_("Language"))Plain inline
<svg>against the page-inlined sprite — paints at first byte, no JavaScript, no layout shift.2. Fragment import — inside a (shadow-DOM) web component.
A shadow root can't reach the page sprite via
<use>, so import the glyph and inline it in your own<svg>.3.
<ol-icon>element — client-side / consumers.For icons inserted from JS. The box is reserved (no shift); the glyph appears once the component upgrades.
What you control: size (
sm/md/lg= 16/20/24px) and color (everything iscurrentColor, so setcoloron the icon or any ancestor). Stroke weight is a fixed 2px — not a knob.Technical
static/icons/src/.make iconsbuilds the sprite, a name manifest, andicons.generated.js(Lit fragments for shadow components). A pre-commit check keeps them in sync.<use href="#name">).Testing
/developers/design#icons— gallery, the "which technique to use" guide, and the<ol-icon>API table all render; the filter works.npm run check:iconspasses (sprite/manifest/module in sync);npm run test:jsgreen.Screenshot
Gallery + usage docs live at
/developers/design#icons. (screenshots to add)Approaches considered and rejected
<use href="sprite.svg#id">): nicer caching, but external<use>is unreliable on the older Safari/iOS in our browserslist — so the sprite is inlined per page instead.<ol-icon>everywhere: a custom element's<use>can't cross a shadow boundary, so shadow-DOM components inline fragments instead; the element is reserved for light-DOM / client-side use.currentColorSVG model, and Iconly's license restricts redistributing source files. Went with Lucide (ISC) plus a small custom set.Stakeholders