Add lucide icons to avatar #2266

joyenjoyer · 2025-11-26T14:54:05Z

Closes INSTUI-4864

Test:
Try all these icon adding options with Avatar and check whether size and color passed to the icon correctly. Also check how changing theme impact the rendered component.

renderIcon={AlarmClockInstUIIcon} // reference to icon
renderIcon={<AlarmClockInstUIIcon />} // icon as React component
renderIcon={()=><AlarmClockInstUIIcon />} // icon as React component returned by an arrow function

github-actions · 2025-11-26T14:58:49Z

PR Preview Action v1.6.3
🚀 View preview at https://instructure.design/pr-preview/pr-2266/
Built to branch `gh-pages` at 2025-12-11 14:10 UTC. Preview will be ready when the GitHub Pages deployment is complete.

packages/__docs__/src/Nav/index.tsx

packages/ui-avatar/src/Avatar/styles.ts

packages/ui-avatar/src/Avatar/index.tsx

joyenjoyer · 2025-12-04T12:38:29Z

packages/ui-avatar/src/Avatar/README.md

-  <Avatar size="large" color="ai" name="AI Assistant" renderIcon={IconAiSolid}  />
-  <Avatar size="x-large" color="ai" name="AI Assistant" renderIcon={IconAiSolid}  />
-  <Avatar size="xx-large" color="ai" name="AI Assistant" renderIcon={IconAiSolid}  />
+  <Avatar size="xx-small" color="ai" name="AI Assistant" renderIcon={IconAiSolid} />


This has to be replaced later to the appropriate Lucide equivalent icon. (Alex is still working on the mapping)

packages/ui-avatar/src/Avatar/styles.ts

matyasf

see my comments

packages/ui-react-utils/src/renderLucideIconWithProps.ts

joyenjoyer · 2025-12-05T09:24:59Z

@matyasf I answered the comments and did changes where they were needed, please check the PR again.

…comments

packages/ui-avatar/src/Avatar/__tests__/Avatar.test.tsx

joyenjoyer · 2025-12-09T22:13:02Z

packages/ui-avatar/src/Avatar/index.tsx

-          color: iconColor
-        })
+        return (
+          <IconPropsProvider size={iconSize} color={iconColor}>


I removed renderLucideIconWithProps since it was too complex to understand and added IconPropsProvider instead. Lucide icons consume size and color through it.

This works with all the wanted patterns:

renderIcon={AlarmClockInstUIIcon} renderIcon={<AlarmClockInstUIIcon />} renderIcon={()=><AlarmClockInstUIIcon />}

joyenjoyer · 2025-12-09T22:14:01Z

packages/ui-avatar/src/Avatar/props.ts

   * When using Lucide icons, Avatar will automatically pass the appropriate size and color props based on the Avatar's size and color.
   */
-  renderIcon?: Renderable<{ size?: string | number; color?: string }>
+  renderIcon?: Renderable


Removed unnecessarily complex typing

joyenjoyer · 2025-12-09T22:15:50Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

+    const contextProps = useIconProps()
+
+    // Merge props: direct props take precedence over context props
+    const finalSize = size ?? contextProps?.size


Followed the React best practice here, component props always override context values by default.

In general I would agree, but this code design explicitly wants to force props if applicable. For example we don't want to have misconfigured Avatars with incorrects sizes or colors. I'd default to context here and if the need arises, we could have a prop like "forceSize" or something later

joyenjoyer · 2025-12-09T22:17:40Z

packages/ui-react-utils/src/IconPropsProvider/IconPropsContext.ts

+ */
+
+import React from 'react'
+import type { IconPropsContextValue } from './IconPropsProvider'


I followed the same file structuring I saw with DeterministicIdProvider

joyenjoyer · 2025-12-09T22:27:05Z

packages/ui-icons-lucide/src/wrapLucideIcon/props.ts

+  | 'inverseColor'
+  | 'disabledBaseColor'
+  | 'disabledOnColor'
+  | 'dark'


Designers confirmed that dark is a valid color, they will use it in the future.

joyenjoyer · 2025-12-09T22:35:13Z

packages/ui-react-utils/src/__tests__/IconPropsProvider.test.tsx

+import { IconPropsProvider, useIconProps } from '../IconPropsProvider'
+
+// Test component that exposes context values
+const TestComponentWithHook = () => {


Mocks are perfectly valid here, since these are unit tests.

matyasf

looks much better now! see my comments

packages/ui-icons-lucide/src/wrapLucideIcon/props.ts

matyasf · 2025-12-10T12:20:35Z

packages/ui-icons-lucide/src/wrapLucideIcon/props.ts

  /**
   * Icon theme color token or CSS color value
   */
  color?: 'inherit' | IconColorToken | string


The old icons use other values ('inherit', 'primary', 'secondary',... How do we handle the migration?

I would add these to the props too, ask for a mapping from design, and do it in the code in index.ts/styles.ts

matyasf · 2025-12-10T12:42:33Z

packages/__docs__/src/Icons/LucideIconsGallery.tsx

                  <code>color</code>: CSS color - e.g.,{' '}
                  <code>&quot;currentColor&quot;</code>


currentColor is not valid AFAIK, a better example would be successColor

matyasf · 2025-12-10T12:43:17Z

packages/__docs__/src/Icons/LucideIconsGallery.tsx

                  <code>&quot;currentColor&quot;</code>
                </li>
                <li>
                  <code>strokeWidth</code>: Number - e.g., <code>2</code>


a better example is sm

matyasf · 2025-12-10T12:43:38Z

packages/__docs__/src/Icons/LucideIconsGallery.tsx

              </Heading>
              <ul>
                <li>
                  <code>size</code>: Number (pixels) - e.g., <code>24</code>


here a semantic example would be better

packages/__docs__/src/Icons/LucideIconsGallery.tsx

matyasf · 2025-12-10T12:48:55Z

packages/ui-icons-lucide/src/wrapLucideIcon/props.ts

 > &
  InstUIIconOwnProps & {
    themeOverride?: ThemeOverrideValue
  } & OtherHTMLAttributes<InstUIIconOwnProps>


Here you could omit the props removed by passtroughProps, I think the syntax is `Omit<OtherHTMLAttributes, 'children', ...>

e.g.

type LucideIconWrapperProps = Omit<LucideProps, 'size' | 'color' | 'strokeWidth' | 'rotate' | 'children' | 'style' | others> & InstUIIconOwnProps & { themeOverride?: ThemeOverrideValue }

matyasf · 2025-12-10T14:00:08Z

packages/ui-avatar/package.json

    "@babel/runtime": "^7.27.6",
    "@instructure/emotion": "workspace:*",
    "@instructure/shared-types": "workspace:*",
    "@instructure/ui-icons": "workspace:*",


is ui-icons still needed?

matyasf · 2025-12-10T14:03:56Z

packages/ui-react-utils/src/IconPropsProvider/IconPropsProvider.tsx

+  size?: string | number
+  color?: string


These have the same type as the props with the same name from wrapLucideIcon/props.ts I think. But in this case we will have to pull in the icons package as a dependency, which might create an impossible build graph... So it might be better to move these where the icons are, we will use them with icons anyway

…w plus add AI gradient coloring

matyasf

Looks good!
I've found just one small visual bug: In the themeOverride prop example the examples are not aligned vertically.

According to AI its because "The children elements all have display: inline-flex and vertical-align: baseline. While they all have the same height (48px), vertical-align: baseline aligns the baselines of the elements. If there is text content or different line-height values within the inline-flex items, or if the parent div has a specific line-height or font-size that interacts with the baseline, it can cause vertical misalignment"

and the fix is pretty simple, just add verticalAlign:'middle to the avatar CSS class

HerrTopi

See my comments. After fixing them, I'd go through it again to check new logic.

I think for the big changes, design needs to be brought in to discuss

HerrTopi · 2025-12-15T23:58:48Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

+    const contextProps = useIconProps()
+
+    // Merge props: direct props take precedence over context props
+    const finalSize = size ?? contextProps?.size


In general I would agree, but this code design explicitly wants to force props if applicable. For example we don't want to have misconfigured Avatars with incorrects sizes or colors. I'd default to context here and if the need arises, we could have a prop like "forceSize" or something later

HerrTopi · 2025-12-15T23:58:59Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

-    }
-
    const styles = useStyle({
      componentId: 'Icon' as const,


Nitpic but this const doesn't do anything here, remove it please so it wont clutter the code

HerrTopi · 2025-12-16T00:01:15Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

      params: {
-        size: semanticSize as InstUIIconOwnProps['size'],
-        color: colorValue,
+        size: finalSize as LucideIconWrapperProps['size'],


This cast isnt't needed

HerrTopi · 2025-12-16T00:05:10Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

-        color: colorValue,
+        size: finalSize as LucideIconWrapperProps['size'],
+        strokeWidth,
+        color: finalColor,


What's the reason behind displayName: LucideIcon(${Icon.displayName || Icon.name}) ` aremt't we sure that displayName (or name) will be available?

Line 83. For some reason I can't comment there

Line 86: const accessibilityProps: Record<string, string | boolean> = {}

| boolean is not needed

HerrTopi · 2025-12-16T00:09:15Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

    )
  }

  WrappedIcon.displayName = `wrapLucideIcon(${Icon.displayName || Icon.name})`


Why is this displayname different from the one in line 83? It should be the same for consistency and theme override's sake

HerrTopi · 2025-12-16T00:10:29Z

packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx

+  return WrappedIcon
 }

 export type { LucideIconWrapperProps, InstUIIconOwnProps }


I don't understand this part. Why re-export these from here? These are already exported from their respected files.

Same for export { default as generateStyle } from './styles'

HerrTopi · 2025-12-16T00:13:53Z

packages/ui-icons-lucide/src/wrapLucideIcon/props.ts

+  | 'actionDestructiveSecondaryActiveColor'

 type InstUIIconOwnProps = {
  /**


Again, I can't comment on unchanged parts, but:
StorkeWidth is a not allowed prop, all stroke width have to be calculated from size and are not changable.
Size van not be number, only the preset tokens.
Same for color. No inherit, no string, only the presets
Ask about these from desing

Calculations and logic are tied to these things in styles.ts, so I won'r review it in depth because it needs to be drastically changed before.

joyenjoyer changed the base branch from master to v12 November 26, 2025 14:54

matyasf assigned joyenjoyer Nov 26, 2025

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from e4f70ef to 44fece3 Compare December 3, 2025 12:22

joyenjoyer commented Dec 3, 2025

View reviewed changes

packages/__docs__/src/Nav/index.tsx Outdated Show resolved Hide resolved

joyenjoyer commented Dec 3, 2025

View reviewed changes

packages/ui-avatar/src/Avatar/styles.ts Show resolved Hide resolved

joyenjoyer commented Dec 3, 2025

View reviewed changes

packages/ui-avatar/src/Avatar/index.tsx Outdated Show resolved Hide resolved

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch 2 times, most recently from 8b5e111 to 1c11f12 Compare December 4, 2025 10:02

joyenjoyer requested review from HerrTopi and matyasf December 4, 2025 10:04

joyenjoyer marked this pull request as ready for review December 4, 2025 10:04

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from 1c11f12 to f75d05f Compare December 4, 2025 12:37

joyenjoyer commented Dec 4, 2025

View reviewed changes

packages/ui-avatar/src/Avatar/styles.ts Outdated Show resolved Hide resolved

feat(ui-avatar): add lucide icons to Avatar

2bc3b92

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from f75d05f to 2bc3b92 Compare December 4, 2025 12:49

matyasf requested changes Dec 4, 2025

View reviewed changes

refactor(ui-react-utils): changes after review

32c91ba

joyenjoyer requested a review from matyasf December 5, 2025 09:24

docs(ui-react-utils,ui-icons-lucide,ui-avatar): add code improvement …

54f0d86

…comments

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch 2 times, most recently from e7c9af6 to 52e6819 Compare December 9, 2025 22:08

joyenjoyer commented Dec 9, 2025

View reviewed changes

packages/ui-avatar/src/Avatar/__tests__/Avatar.test.tsx Outdated Show resolved Hide resolved

joyenjoyer commented Dec 9, 2025

View reviewed changes

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from 52e6819 to 8aae67a Compare December 9, 2025 22:26

joyenjoyer commented Dec 9, 2025

View reviewed changes

refactor(ui-icons-lucide,ui-avatar): review fixes

543b72a

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from 8aae67a to 543b72a Compare December 9, 2025 22:30

joyenjoyer commented Dec 9, 2025

View reviewed changes

matyasf requested changes Dec 10, 2025

View reviewed changes

refactor(ui-react-utils,ui-icons-lucide,ui-avatar): fixes after revie…

7e5aaeb

…w plus add AI gradient coloring

joyenjoyer force-pushed the add_lucide_icons_to_avatar branch from 27b73ca to 7e5aaeb Compare December 11, 2025 14:03

joyenjoyer requested review from matyasf December 11, 2025 14:04

matyasf requested changes Dec 11, 2025

View reviewed changes

HerrTopi requested changes Dec 16, 2025

View reviewed changes

		<code>color</code>: CSS color - e.g.,{' '}
		<code>"currentColor"</code>

Add lucide icons to avatar #2266

Are you sure you want to change the base?

Add lucide icons to avatar #2266

Uh oh!

Conversation

joyenjoyer commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Built to branch gh-pages at 2025-12-11 14:10 UTC. Preview will be ready when the GitHub Pages deployment is complete.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyenjoyer commented Dec 5, 2025

Uh oh!

Uh oh!

joyenjoyer Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyenjoyer Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyenjoyer Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

HerrTopi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyenjoyer commented Nov 26, 2025 •

edited

Loading

github-actions bot commented Nov 26, 2025 •

edited

Loading

Built to branch `gh-pages` at 2025-12-11 14:10 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

joyenjoyer Dec 9, 2025 •

edited

Loading

joyenjoyer Dec 9, 2025 •

edited

Loading

joyenjoyer Dec 9, 2025 •

edited

Loading