chore(svelte): Migrate all icons to Lucide/custom icons#63246
Conversation
6b19b5e to
90fe8f8
Compare
| width: $icon-size; | ||
| height: $icon-size; | ||
| color: var(--icon-fill-color, var(--icon-color, inherit)); | ||
| color: var(--icon-fill-color, var(--color, inherit)); |
There was a problem hiding this comment.
This brings in line with how the original Icon component works. But we should probably rethink the API for colorization, I find it a bit confusing.
There was a problem hiding this comment.
Ah, nice. I ran into this adding an icon in another PR I'm working on. Basically, --icon-color is set at the root, so inherit was never triggering. Solved it before I even got to ask about it 😄
There was a problem hiding this comment.
This was renamed from Icon2.
There was a problem hiding this comment.
This was renamed from Icon.
| <div class="header"> | ||
| <Button variant="icon" on:click={toggle} aria-label="{expanded ? 'Hide' : 'Show'} file diff"> | ||
| <Icon inline svgPath={expanded ? mdiChevronDown : mdiChevronRight} /> | ||
| <Icon inline icon={expanded ? ILucideChevronDown : ILucideChevronRight} /> |
There was a problem hiding this comment.
I feel like we should have a dedicated component for this. We use this quite often.
There was a problem hiding this comment.
This file wasn't used anymore.
camdencheek
left a comment
There was a problem hiding this comment.
Woot! Thanks for cleaning this up!
| "//:node_modules/@codemirror/search", | ||
| "//:node_modules/@codemirror/state", | ||
| "//:node_modules/@codemirror/view", | ||
| "//:node_modules/@mdi/js", |
There was a problem hiding this comment.
I'll have to re-add it for the context specific suggestions 🤷♂️
|
|
||
| export let title: string | ||
| export let svgIconPath: string = '' | ||
| export let icon: ComponentProps<Icon>['icon'] |
There was a problem hiding this comment.
Oh, that's a much nicer type name than what I came up with when I attempted this
There was a problem hiding this comment.
Q: why not use the exported IconComponent type from Icon?
There was a problem hiding this comment.
I thought it makes to use this type in Svelte components where the Icon component is actually used, and use the dedicated type in TS files where we just define other types.
| <Icon svgPath={icons[severity]} --icon-size="16px" --color={isError ? 'var(--danger)' : 'var(--text-title)'} /> | ||
| <Icon | ||
| icon={icons[severity]} | ||
| aria-label={severity} |
There was a problem hiding this comment.
Thank you for auditing the aria properties while you're at it!
There was a problem hiding this comment.
I could have done more but I didn't want to stuff too much into a single PR
| import type { RepositoryGitRevAuthor } from './RepositoryRevPicker.gql' | ||
|
|
||
| export let iconPath: string | ||
| export let icon: ComponentProps<Icon>['icon']|undefined = undefined |
There was a problem hiding this comment.
Similarly, why not use IconComponent?
Closes srch-467
tl;dr: This commit switches all remaining mdi icons to lucide.
This commit does a couple of things to make the migration to lucide icons and to the
Icon2(nowIconagain) component complete:unplugin-icons custom collections support to reference themIconcomponent toSVGIcon. We still need it for integrating with shared web app code.Icon2component toIcon.<svelte:componentwhich is used by (now)Icon.I put our generic icons in
assets/icons/and the specific symbol icons inassets/symbol/icons. They can be referenced viaISgNameandISymbolName. I'm open to changes to the location and the naming.If the same icon wasn't available in lucide I tried to find one that made sense semantically. I expect we'll make another pass (in a separate PR) to adjust icons and tweak icon sizes/colors.
Test plan
pnpm buildand svelte check