refactor: replace carbon icons#1454
Conversation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/components/Package/Versions.vue (2)
441-446: Redundantanimate-spinon pre-animated svg-spinner icon.The
i-svg-spinners:ring-resizeicon is already animated within the SVG itself. Addingmotion-safe:animate-spinapplies a second CSS rotation on top, which is redundant and may cause visual artifacts.♻️ Suggested fix
<span v-if="loadingTags.has(row.tag)" - class="i-svg-spinners:ring-resize w-3 h-3 motion-safe:animate-spin" + class="i-svg-spinners:ring-resize w-3 h-3" data-testid="loading-spinner" aria-hidden="true" />
596-601: Same redundantanimate-spinas noted above.♻️ Suggested fix
<span v-if="otherVersionsLoading" - class="i-svg-spinners:ring-resize w-3 h-3 motion-safe:animate-spin" + class="i-svg-spinners:ring-resize w-3 h-3" data-testid="loading-spinner" aria-hidden="true" />app/components/VersionSelector.vue (1)
471-487: Remove per-button focus-visible utilities and rely on the global rule.The trigger button still applies inline focus-visible classes. These should be removed so the global button focus style in
main.cssremains the single source of truth.Based on learnings: “In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70.”Proposed change
- class="flex items-center gap-1.5 text-fg-subtle font-mono text-sm hover:text-fg transition-[color] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-bg rounded" + class="flex items-center gap-1.5 text-fg-subtle font-mono text-sm hover:text-fg transition-[color] rounded"app/components/Package/InstallScripts.vue (1)
94-103: Drop the per-button focus-visible utility here.Use the global focus-visible styling instead of inline focus-visible classes on this button.
Based on learnings: “In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70.”Proposed change
- class="flex items-center gap-1.5 text-xs text-fg-muted hover:text-fg transition-colors duration-200 focus-visible:outline-accent/70 rounded" + class="flex items-center gap-1.5 text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded"app/components/Org/TeamsPanel.vue (1)
267-276: Remove inline focus-visible utilities on buttons.This button still applies per-element focus-visible styling; rely on the global rule instead (and apply the same cleanup to other buttons in this component).
Based on learnings: “In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70.”Proposed change
- class="p-1.5 text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70" + class="p-1.5 text-fg-muted hover:text-fg transition-colors duration-200 rounded"
danielroe
left a comment
There was a problem hiding this comment.
this is tricky. in general I think I preferred carbon to lucide but agree fewer icon sets is preferable
thank you ❤️
|
@essenmitsosse Wow, that's great! A few minor thoughts Let's replace i-lucide:circle-x to i-lucide:x - all of our buttons have a border and now it is a round border inside a rounded border. Specifically this accessibility icon on mobile burger looks a bit weird in this context for me... Would be better to change dependency tree icon to something different Missed folder icon, it should be generated from a file with all the icons for file tree. The problem might be that there's a hyphen in one place and a colon in another. |
I tried the other way around, but carbon has much more stuff missing, while lucide seems pretty comprehensive. On a personal note: I find carbon to cold and technical. I know opinions might differ. But having them mixed isn't ideal either for consistancy.
✅
It's the same icon as the view we are linking to. Alas there is no galaxy icon in lucide. Honestly: I never was a big fan of using icons here anyway. I feel like this needs some other UI pattern, because you are basically clicking on some abstract icon and see where you are taken. Maybe we can leave it for now and try to find a generally better solution? Also I think the tooltip is wrong, because that doesn't link to a dependency tree?
I tried something. I am not sure if it is correct, that the sprites don't get put into Edit: works now. |
|
@essenmitsosse now everything looks nice, thanks About a11y - well, maybe that's okay and just mine view. Do you like the icon in general? |
|
Also noticed building custom icons is super easy in the lucide style. So if we need some custom stuff later on, like having icon XYZ but with a small plus on the side, we can should build it ourselves. |
|
Great, I'll keep that in mind - I usually just cut everything together and trying to fix Thanks again for clean changes, perfect PR ❤️ |
In npmx-dev#1454, Carbon icons were removed. This caused a conflict when later that day npmx-dev#1417 was merged. Additionally, in npmx-dev#1454 some non-existing icons were used, like x-filled, or check-filled.
that aged like milk |
|
And it's fine - it's too easy to lose a few small changes in such a flux We'll try to think about how to check site better from global and conflicted changes, after the vacation. Maybe we might review the site live in the evenings 🤔 |










This replaces all carbon icons (mostly with lucide, some with simple-icons) and drops carbon completely.
I noticed that basically all carbon icons can easily be replaced with lucide icons. Most of them are almost identical, some of them are even clearer in my opinion (carbon has some really weird hyper specific icons), and generally I think they look a little less pleasant. Also in some places we were using the same icon from either set.
I know this is pretty opinionated, but I think reducing the dependencies is always a good thing, and this makes it also a little easier to pick an icon, because there would be only one general icons set the choose from. And for brand icons there is now only simple-icons.
The only icon where I had to freestyle the replacement is the "no dependency icon"
before/after
other screenshots