docs: document custom SVG icons without a gulp build step - #794
Merged
Conversation
customize.md only described the src/icons -> gulp build-icons -> gulp css icon-font pipeline, which is meant for icons bundled into this package's own build/fork. It never mentioned that the icon option already supports plain CSS classes or a function injecting inline SVG, so users following the docs hit an unnecessary build/fork requirement for something already possible. Fixes #762
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab6794ae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // or inject an inline <svg> (or <img>) directly into the item and | ||
| // return a class name to mark it as done (see the icon option docs) | ||
| icon: function (opt, $itemElement) { | ||
| $itemElement.prepend('<svg class="context-menu-icon" ...>...</svg>'); |
There was a problem hiding this comment.
When this menu is first shown and whenever it is subsequently updated or reopened, op.update invokes the icon callback again, so this unconditional prepend adds another SVG each time; the item already receives one during op.create, meaning even its first display contains duplicates and repeated openings continually grow the DOM. Make the callback idempotent, such as by checking for/removing the previous inline icon before prepending it.
Useful? React with 👍 / 👎.
bbrala
added a commit
that referenced
this pull request
Jul 30, 2026
* docs: make the inline SVG icon example idempotent The example added in #794 prepended an <svg> unconditionally: icon: function (opt, $itemElement) { $itemElement.prepend('<svg class="context-menu-icon" ...>...</svg>'); return 'context-menu-icon-inline'; } A callback `icon` is invoked twice before the menu is ever visible - once from op.create() and again from the op.update() that op.show() runs on every show - and once more on every subsequent open or update. The prepend has no guard, so the item already carries two icons on its first display and gains another with every open. The returned 'context-menu-icon-inline' class reads as a done-marker, but nothing ever checked it. Guard on the injected element instead, which is what makes the callback safe to re-run. Two related doc fixes: * The example put `context-menu-icon` on the child <svg>. That is the plugin's own item-level class (classNames.icon), which carries an absolutely positioned ::before from the base-context-menu-icon mixin. Use a class of your own instead. * items.md never said the callback re-runs, which is what allowed the example to be written this way. Say so, and point at the guard pattern. The canonical example there needs no guard because $itemElement.html() replaces content rather than adding to it. Reported by Codex review on #794, after the PR had already been merged. * fix: replace the previous class when a callback icon is re-evaluated Addresses the Codex review point on the docs change in this PR: the new guidance said a callback `icon` re-runs "so that the icon can reflect current state", but a callback whose class tracks that state did not actually work. op.update() removed `item._icon` before re-invoking the callback, then applied the result without ever storing it. `item._icon` therefore stayed at whatever op.create() had produced, so only that very first class was ever removed and every class returned after it stayed on the item: create -> _icon='state-one', item has state-one update -> removeClass('state-one'), addClass('state-two') _icon unchanged update -> removeClass('state-one') no-op, addClass('state-three') item now carries state-two AND state-three Assign the result back to `item._icon` so the next update removes the class the previous one applied. Rather than documenting the limitation, which would enshrine it, this makes the documented contract true. The alternative was to soften the docs and leave a callback `icon` unable to express changing state at all, which is the whole reason it is re-evaluated. Backwards compatibility: a callback returning a constant string - the common case by far - is unaffected, since removing and re-adding the same class is a no-op either way. Only a callback returning *different* strings changes, and only by dropping stale classes, which is the bug. A callback returning an element rather than a string is unaffected: `removeClass()` ignores a non-string argument, exactly as it did before. `item._icon` is internal and read only in op.create() and op.update(). Also documents that a returned class is swapped on each call, while anything the callback does to $itemElement itself is not, so only that part needs to be idempotent.
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.
Summary
Addresses #762. The reporter (and
splitbrainin the comments) wanted to use their own SVG icons without going through thesrc/icons→gulp build-icons→gulp csspipeline, which is meant for icons bundled into this package's own build/fork — not something most consumers should need to do.That's already possible today: the
iconoption accepts either a plain CSS class (which you define yourself, e.g. viabackground-image) or a function that can inject inline SVG markup directly into the item.customize.mdjust never mentioned this, so it only pointed people at the heavyweight build/fork path.This PR adds a short "Using your own SVG icons without a build step" section documenting both approaches, matching the existing icon-function example style already used in
items.md.(The npm-install/build breakage originally reported in #762 was unrelated tooling drift and is already fixed on current master — verified separately.)
Test plan
npm run docs:build— site builds cleanly, new section renders correctly indocumentation/_site/docs/customize.html