Conversation
joyenjoyer
force-pushed
the
INSTUI-4892-tidy-up-the-icons-gallery
branch
2 times, most recently
from
84fae89 to
feb4960
Compare
|
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
docs/guides/upgrade-guide.md
Outdated
| 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?
There was a problem hiding this comment.
Yes, old icons will be shown on a separate page than new 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?
There was a problem hiding this comment.
This file is generated by the generateCustomIndex script. It contains all the custom icons that developers can import.
| // 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.
Naming conflicts are handled in generateLucideIndex (lines 69-88). Custom icons shadow Lucide icons of the same name.
There was a problem hiding this comment.
isn't this the canvas logo?
There was a problem hiding this comment.
I copied this icon from the old icon build system. Yeah it looks like the canvas logo and canvas.svg looks pretty similar. I reach out to designers whether they want to keep this under this name.
| 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, |
There was a problem hiding this comment.
I don't like it either but it is needed for the type IconInfo otherwise ts throws an error.
packages/ui-codemods/lib/index.ts
Outdated
| 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 | |||
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?
There was a problem hiding this comment.
This name conversion is needed because export names can't contain hyphens (i.e. ai-infoInstUIIcon) also React component names must start with uppercase.
| 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. |
joyenjoyer
force-pushed
the
INSTUI-4892-tidy-up-the-icons-gallery
branch
from
feb4960 to
a678ae1
Compare
3 participants
No description provided.