Add panel colored shapes behind shuttle icons in all arcana#2817
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds dynamic eager-loading of Arcanum SVG icons (matching Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/core/internal/IconGallery.tsx`:
- Around line 429-439: The Arcanum slider label is missing the visible "Size:"
text; update the label that wraps the input (the block using arcanumSize() and
setArcanumSize()) to include a visible span "Size:" before the range input
(matching the primary slider's pattern) so users know what the control adjusts;
keep the same styling/utility classes used by the other slider's label/span to
ensure visual consistency with the primary icon-size slider.
- Around line 442-460: The Arcanum items are only coloring the SVG via color:
getColorStyle() but do not render a filled panel behind each icon; update the
ARCANUM_ICONS rendering so the icon has a separate backing element (e.g., create
a sibling or absolutely positioned child inside the container) that uses
background or backgroundColor: getColorStyle() and the same width/height derived
from arcanumSize(), while keeping the SVG colored via color: getColorStyle() (or
vice versa depending on the SVGs). Target the inner container that currently
sets style with getColorStyle()/arcanumSize() and icon.component, and ensure the
backing element has the desired shape (square/rounded) and appropriate
z-index/padding so the icon sits visibly on top.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b84f5af2-0ca8-4ade-8a46-9a169e83ad3a
⛔ Files ignored due to path filters (6)
js/app/packages/macro-icons/arcanum/arcanum-002.svgis excluded by!**/*.svgjs/app/packages/macro-icons/arcanum/arcanum-003.svgis excluded by!**/*.svgjs/app/packages/macro-icons/arcanum/arcanum-005.svgis excluded by!**/*.svgjs/app/packages/macro-icons/arcanum/arcanum-006.svgis excluded by!**/*.svgjs/app/packages/macro-icons/arcanum/arcanum-009.svgis excluded by!**/*.svgjs/app/packages/macro-icons/arcanum/arcanum-010.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
js/app/packages/core/internal/IconGallery.tsx
| <label class="flex items-center gap-2 text-xs text-ink"> | ||
| <input | ||
| type="range" | ||
| min="10" | ||
| max="400" | ||
| value={arcanumSize()} | ||
| onInput={(e) => setArcanumSize(Number(e.currentTarget.value))} | ||
| class="icon-gallery-slider w-24" | ||
| /> | ||
| <span class="w-8 text-[10px] text-muted">{arcanumSize()}px</span> | ||
| </label> |
There was a problem hiding this comment.
Add a "Size:" label to the arcanum slider for UI consistency.
The primary icon-size slider above (lines 304–315) is prefixed with a <span>Size:</span> inside its <label>. The new Arcanum slider wraps the <input> in a label but omits the visible text, so there's no textual affordance telling the user what the range controls.
Proposed fix
<label class="flex items-center gap-2 text-xs text-ink">
+ <span>Size:</span>
<input
type="range"
min="10"
max="400"
value={arcanumSize()}
onInput={(e) => setArcanumSize(Number(e.currentTarget.value))}
class="icon-gallery-slider w-24"
/>
<span class="w-8 text-[10px] text-muted">{arcanumSize()}px</span>
</label>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <label class="flex items-center gap-2 text-xs text-ink"> | |
| <input | |
| type="range" | |
| min="10" | |
| max="400" | |
| value={arcanumSize()} | |
| onInput={(e) => setArcanumSize(Number(e.currentTarget.value))} | |
| class="icon-gallery-slider w-24" | |
| /> | |
| <span class="w-8 text-[10px] text-muted">{arcanumSize()}px</span> | |
| </label> | |
| <label class="flex items-center gap-2 text-xs text-ink"> | |
| <span>Size:</span> | |
| <input | |
| type="range" | |
| min="10" | |
| max="400" | |
| value={arcanumSize()} | |
| onInput={(e) => setArcanumSize(Number(e.currentTarget.value))} | |
| class="icon-gallery-slider w-24" | |
| /> | |
| <span class="w-8 text-[10px] text-muted">{arcanumSize()}px</span> | |
| </label> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/internal/IconGallery.tsx` around lines 429 - 439, The
Arcanum slider label is missing the visible "Size:" text; update the label that
wraps the input (the block using arcanumSize() and setArcanumSize()) to include
a visible span "Size:" before the range input (matching the primary slider's
pattern) so users know what the control adjusts; keep the same styling/utility
classes used by the other slider's label/span to ensure visual consistency with
the primary icon-size slider.
| <div class="flex flex-wrap gap-3"> | ||
| <For each={ARCANUM_ICONS}> | ||
| {(icon) => ( | ||
| <div class="inline-flex flex-col items-center rounded-[1px] border border-edge-muted p-2"> | ||
| <div | ||
| class="flex items-center justify-center" | ||
| style={{ | ||
| color: getColorStyle(), | ||
| width: `${arcanumSize()}px`, | ||
| height: `${arcanumSize()}px`, | ||
| }} | ||
| > | ||
| <icon.component /> | ||
| </div> | ||
| <span class="mt-2 text-[8px] text-muted">{icon.name}</span> | ||
| </div> | ||
| )} | ||
| </For> | ||
| </div> |
There was a problem hiding this comment.
PR objective not implemented: no "panel colored shape" is rendered behind the arcanum icons.
The PR title is "Add panel colored shapes behind shuttle icons in all arcana", and the AI summary describes "each rendered inside a colored, square container". In the actual markup, the inner div only sets color: getColorStyle() (which maps to SVG currentColor for the icon's fill/stroke) — there is no background/background-color or any other shape behind the icon. The container's only visible chrome is the existing border border-edge-muted p-2 used by the other galleries.
If the goal is to show the selected swatch as a filled panel behind each shuttle icon (as the title implies), the Arcanum item needs its own backing element, e.g.:
Proposed fix
<div class="flex flex-wrap gap-3">
<For each={ARCANUM_ICONS}>
{(icon) => (
<div class="inline-flex flex-col items-center rounded-[1px] border border-edge-muted p-2">
<div
- class="flex items-center justify-center"
+ class="flex items-center justify-center"
style={{
- color: getColorStyle(),
+ background: getColorStyle(),
width: `${arcanumSize()}px`,
height: `${arcanumSize()}px`,
}}
>
<icon.component />
</div>
<span class="mt-2 text-[8px] text-muted">{icon.name}</span>
</div>
)}
</For>
</div>(Or keep color for the icon and add a separate background — whichever the arcanum SVGs expect.)
Please confirm whether the colored "shape behind" was intended to land in this PR, or if it's deferred to a follow-up; if deferred, consider updating the title/description so reviewers aren't misled.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="flex flex-wrap gap-3"> | |
| <For each={ARCANUM_ICONS}> | |
| {(icon) => ( | |
| <div class="inline-flex flex-col items-center rounded-[1px] border border-edge-muted p-2"> | |
| <div | |
| class="flex items-center justify-center" | |
| style={{ | |
| color: getColorStyle(), | |
| width: `${arcanumSize()}px`, | |
| height: `${arcanumSize()}px`, | |
| }} | |
| > | |
| <icon.component /> | |
| </div> | |
| <span class="mt-2 text-[8px] text-muted">{icon.name}</span> | |
| </div> | |
| )} | |
| </For> | |
| </div> | |
| <div class="flex flex-wrap gap-3"> | |
| <For each={ARCANUM_ICONS}> | |
| {(icon) => ( | |
| <div class="inline-flex flex-col items-center rounded-[1px] border border-edge-muted p-2"> | |
| <div | |
| class="flex items-center justify-center" | |
| style={{ | |
| background: getColorStyle(), | |
| width: `${arcanumSize()}px`, | |
| height: `${arcanumSize()}px`, | |
| }} | |
| > | |
| <icon.component /> | |
| </div> | |
| <span class="mt-2 text-[8px] text-muted">{icon.name}</span> | |
| </div> | |
| )} | |
| </For> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/internal/IconGallery.tsx` around lines 442 - 460, The
Arcanum items are only coloring the SVG via color: getColorStyle() but do not
render a filled panel behind each icon; update the ARCANUM_ICONS rendering so
the icon has a separate backing element (e.g., create a sibling or absolutely
positioned child inside the container) that uses background or backgroundColor:
getColorStyle() and the same width/height derived from arcanumSize(), while
keeping the SVG colored via color: getColorStyle() (or vice versa depending on
the SVGs). Target the inner container that currently sets style with
getColorStyle()/arcanumSize() and icon.component, and ensure the backing element
has the desired shape (square/rounded) and appropriate z-index/padding so the
icon sits visibly on top.
1 participant
No description provided.