Conversation
joyenjoyer
force-pushed
the
INSTUI-4892-tidy-up-the-icons-gallery
branch
from
69c5e5e to
84fae89
Compare
joyenjoyer
force-pushed
the
INSTUI-4892-tidy-up-the-icons-gallery
branch
from
84fae89 to
feb4960
Compare
|
balzss
left a comment
balzss
left a comment
There was a problem hiding this comment.
skimmed through the PR and mostly left questions, not change requests since i'm not familiar with the first iteration at all
i'm not really sure about the name "stroke" for the new icon variants, since it's a small technical detail about them, not really important for neither us and especially not for the end user. i think a more semantic differentiation could be better but that's just my 2 cents
as someone who is not familiar with how the icons are generated, from where and why, the structure feels very complicated to me and i'm not really looking forward to maintain this in the future. i think it would really benefit us in the long run to come up with something that's easier to wrap our heads around
also with the old icons we also provided the raw svg values along with the react components (in the docs page). are those not needed anymore? I'm not against it, just asking if that is intentional
i also don't see the colored ai icon in the new custom icons, was that intentional too? don't we have components depending on that?
| InstUI has switched to a new icon set based on [Lucide](https://lucide.dev/icons/). We are still keeping some Instructure-specific icons, like product logos. We have a codemod that will help you migrate your code to the new icon set (see below). | ||
|
|
||
| ### Lucide Icons Package | ||
| ### Stroke-Based Icons Package |
There was a problem hiding this comment.
where is this name coming from? did design come up with it? it's a bit confusing for me what it means (and how this is different from other icons. others are store based too, no?)
| const __dirname = path.dirname(__filename) | ||
| const require = createRequire(import.meta.url) | ||
|
|
||
| // Use CommonJS require to load ui-themes to avoid ES module resolution issues |
There was a problem hiding this comment.
is this a new issue? or was it not working before?
There was a problem hiding this comment.
Is there a reason this is in a separate dir and not under Icons?
| * Stroke width is automatically derived from size for consistent visual weight. | ||
| * Numeric and custom CSS values are not supported. | ||
| */ | ||
| export function wrapStrokeIcon( |
There was a problem hiding this comment.
wouldn't wrapLucideIcon actually be an appropriate name here? since this is actually for wrapping just the lucide library icons
There was a problem hiding this comment.
is this a build file? seems like it has the same svg data as svg/Custom/.... or what's the difference?
| // Re-export all generated SVG icons and Lucide icons | ||
| // Main entry point - exports all icons for backwards compatibility | ||
| export * from './lucide' | ||
| export * from './custom' |
There was a problem hiding this comment.
could this (in theory) cause a naming conflict if a custom icon has a same name as a lucide icon? is that even a problem?
There was a problem hiding this comment.
isn't this the canvas logo?
| const MyIcon = () => { | ||
| return ( | ||
| <${iconName} size={'2xl'} color='successColor'/> | ||
| <${icon.name} size={'2xl'} color='successColor' /> |
There was a problem hiding this comment.
nitpick: double quotes and string primitive would be nicer here
|
|
||
| function getUsageInfo(iconName: string) { | ||
| return `import { ${iconName} } from '@instructure/ui-icons' | ||
| // Get all stroke icons |
There was a problem hiding this comment.
this section is a bit overcomplicated. you are getting the keys of each icon type to convert an object list to a string array and then you are converting it back to an object while accessing each item in the original list again. this is resource heavy and with lots of icons could cause perf issue.
| ...strokeIconNames.map((name) => ({ | ||
| name, | ||
| component: (StrokeIcons as any)[name], | ||
| source: 'stroke' as const, |
| migrateToNewIcons, | ||
| // Backwards compatibility alias (deprecated) | ||
| migrateToNewIcons as migrateToLucideIcons |
There was a problem hiding this comment.
Why is here this exported twice, once under an alias?
What do you mean by "Backwards compatibility alias (deprecated)"?
| @@ -1,38 +0,0 @@ | |||
| ## ui-codemods | |||
| interface MappingData { | ||
| version: string | ||
| lastUpdated: string | ||
| mappings: IconMapping[] | ||
| unmapped?: Array<{ | ||
| instUI: { | ||
| line: string | ||
| solid: string | ||
| } | ||
| reason: string | ||
| }> | ||
| } |
There was a problem hiding this comment.
This type has nothing to do with the actual data its applied to?
| console.error( | ||
| `Mapped ${mappingData.mappings.length} InstUI icons to Lucide` | ||
| ) |
There was a problem hiding this comment.
This never happens. Also please dont use console.error to indicate success
| const svgFiles = fs | ||
| .readdirSync(svgDir) | ||
| .filter((f) => f.endsWith('.svg')) | ||
| .sort() |
There was a problem hiding this comment.
Why does it need to be sorted?
| /** | ||
| * Convert kebab-case filename to PascalCase icon name | ||
| * Examples: | ||
| * ai-info.svg -> AiInfo | ||
| * post-sis.svg -> PostSis | ||
| * calculator-desmos.svg -> CalculatorDesmos | ||
| */ | ||
| function fileNameToIconName(fileName: string): string { | ||
| return fileName | ||
| .replace('.svg', '') | ||
| .split('-') | ||
| .map((part) => part.charAt(0).toUpperCase() + part.slice(1)) | ||
| .join('') | ||
| } |
There was a problem hiding this comment.
Why not just call them by the converted name like AiInfo?
| attributes[camelCaseName] = 'currentColor' | ||
| } else if (camelCaseName === 'stroke' && value !== 'none') { | ||
| hasStroke = true | ||
| attributes[camelCaseName] = 'currentColor' |
There was a problem hiding this comment.
Fixes like this should be in the .svg files, not patched every time when builing?
| } else if (camelCaseName === 'strokeWidth') { | ||
| attributes[camelCaseName] = '1.5' |
There was a problem hiding this comment.
I think this code never ececutes, why is it here? also why 1.5?
| if ( | ||
| tagName === 'path' || | ||
| tagName === 'polygon' || | ||
| tagName === 'circle' || | ||
| tagName === 'rect' || | ||
| tagName === 'ellipse' | ||
| ) { | ||
| if (!('fill' in attributes) && !('stroke' in attributes)) { | ||
| attributes.fill = 'currentColor' | ||
| hasFill = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, we should not process these svgs on every build, but once commit in the right format
| } | ||
|
|
||
| const iconType = shouldUseFilled ? '(filled)' : '(stroke)' | ||
| console.error(` ✓ ${fileName} → ${iconName}InstUIIcon ${iconType}`) |
There was a problem hiding this comment.
please do not use console.error for success messages
| const iconType = shouldUseFilled ? '(filled)' : '(stroke)' | ||
| console.error(` ✓ ${fileName} → ${iconName}InstUIIcon ${iconType}`) | ||
| } catch (error) { | ||
| console.error(` ✗ Error: ${fileName}:`, error) |
There was a problem hiding this comment.
I would throw an error in this case, the build should fail
| export function wrapFilledIcon( | ||
| iconNode: IconNode, | ||
| iconName: string, | ||
| viewBox: string = '0 0 24 24' |
There was a problem hiding this comment.
why is there a default value for this??
|
|
||
| // Extract width and height from viewBox for gradient sizing | ||
| const viewBoxParts = viewBox.split(' ') | ||
| const gradientSize = parseFloat(viewBoxParts[2]) || 24 |
There was a problem hiding this comment.
why the default value? When is it applied?
| @@ -0,0 +1,145 @@ | |||
| /* | |||
There was a problem hiding this comment.
Is tis the same file as custom/wrapFilledIcon? Why do you need this code duplication?
| export function createIconWrapper( | ||
| config: IconWrapperConfig | ||
| ): React.ComponentType<StrokeIconWrapperProps> { | ||
| const { displayName, renderIcon, renderGradientIcon } = config |
There was a problem hiding this comment.
This is so complicated now
balzss
left a comment
balzss
left a comment
There was a problem hiding this comment.
also it seems like the custom icons with ai color have a border around them:
<div>
<IconButton><SparklesInstUIIcon size="2xl" color="ai"/></IconButton>
<IconButton><Sparkles2InstUIIcon size="2xl" color="ai" /></IconButton>
</div>
is this intentional?
also the new icons docs page have a border around which doesn't seem necessary and the focus ring is cut off from the input field:
the custom filled ai button is black in the avatar example, this also seems like a bug:
|
Also color="ai" seems to mess up lines, e.g. |
3 participants
No description provided.