Skip to content

Commit 8aae67a

Browse files
joyenjoyerjoyenjoyer
committed
refactor(ui-icons-lucide,ui-avatar): review fixes
1 parent 54f0d86 commit 8aae67a

File tree

15 files changed

+551
-926
lines changed

15 files changed

+551
-926
lines changed

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

Lines changed: 14 additions & 241 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ import { runAxeCheck } from '@instructure/ui-axe-check'
2929
import '@testing-library/jest-dom'
3030
import Avatar from '../index'
3131
import { IconGroupLine } from '@instructure/ui-icons'
32-
import {
33-
UserInstUIIcon,
34-
CircleUserInstUIIcon
35-
} from '@instructure/ui-icons-lucide'
32+
import { HeartInstUIIcon } from '@instructure/ui-icons-lucide'
3633

3734
describe('<Avatar />', () => {
3835
describe('for a11y', () => {
@@ -82,14 +79,9 @@ describe('<Avatar />', () => {
8279
})
8380

8481
describe('when the renderIcon prop is provided', () => {
85-
it('should display an svg passed', async () => {
86-
const SomeIcon = () => (
87-
<svg>
88-
<circle cx="25" cy="75" r="20" />
89-
</svg>
90-
)
82+
it('should display a Lucide icon when passed as component reference', async () => {
9183
const { container } = render(
92-
<Avatar name="avatar name" renderIcon={SomeIcon}>
84+
<Avatar name="avatar name" renderIcon={HeartInstUIIcon}>
9385
hello
9486
</Avatar>
9587
)
@@ -107,249 +99,30 @@ describe('<Avatar />', () => {
10799
expect(avatarSvg).toBeInTheDocument()
108100
})
109101

110-
it('should pass the correct size and color props to icon based on Avatar size', async () => {
111-
const MockIcon = vi.fn((props: any) => (
112-
<svg
113-
data-testid="mock-icon"
114-
data-size={props.size}
115-
data-color={props.color}
116-
>
117-
<circle cx="25" cy="75" r="20" />
118-
</svg>
119-
))
120-
;(MockIcon as any).displayName = 'wrapLucideIcon(MockIcon)' // TODO why mock the icon? Why not use a real one?
121-
122-
const { container } = render(
123-
<Avatar name="avatar name" size="medium" renderIcon={MockIcon} />
124-
)
125-
126-
expect(MockIcon).toHaveBeenCalledWith(
127-
expect.objectContaining({ size: 'md', color: expect.any(String) })
128-
)
129-
const icon = container.querySelector('[data-testid="mock-icon"]')
130-
expect(icon).toHaveAttribute('data-size', 'md')
131-
expect(icon).toHaveAttribute('data-color')
132-
})
133-
134-
it('should map xx-small Avatar to xs icon size', async () => {
135-
const MockIcon = vi.fn(() => (
136-
<svg data-testid="mock-icon">
137-
<circle cx="25" cy="75" r="20" />
138-
</svg>
139-
))
140-
;(MockIcon as any).displayName = 'wrapLucideIcon(MockIcon)'
141-
142-
render(
143-
<Avatar name="avatar name" size="xx-small" renderIcon={MockIcon} />
144-
)
145-
146-
expect(MockIcon).toHaveBeenCalledWith(
147-
expect.objectContaining({ size: 'xs', color: expect.any(String) })
148-
)
149-
})
150-
151-
it('should map x-small Avatar to xs icon size', async () => {
152-
const MockIcon = vi.fn(() => (
153-
<svg data-testid="mock-icon">
154-
<circle cx="25" cy="75" r="20" />
155-
</svg>
156-
))
157-
;(MockIcon as any).displayName = 'wrapLucideIcon(MockIcon)'
158-
159-
render(<Avatar name="avatar name" size="x-small" renderIcon={MockIcon} />)
160-
161-
expect(MockIcon).toHaveBeenCalledWith(
162-
expect.objectContaining({ size: 'xs', color: expect.any(String) })
163-
)
164-
})
165-
166-
it('should work with icons that ignore the size prop (backwards compatibility)', async () => {
167-
const IconWithoutSize = () => (
168-
<svg data-testid="icon-without-size">
169-
<circle cx="25" cy="75" r="20" />
170-
</svg>
171-
)
172-
173-
const { container } = render(
174-
<Avatar name="avatar name" size="large" renderIcon={IconWithoutSize} />
175-
)
176-
177-
const icon = container.querySelector('[data-testid="icon-without-size"]')
178-
expect(icon).toBeInTheDocument()
179-
})
180-
181-
it('should display a Lucide icon with default size', async () => {
182-
const { container } = render(
183-
<Avatar name="avatar name" renderIcon={UserInstUIIcon} />
184-
)
185-
186-
const avatarSvg = container.querySelector('svg')
187-
expect(avatarSvg).toBeInTheDocument()
188-
})
189-
190-
it('should display a Lucide icon with medium Avatar size', async () => {
191-
const { container } = render(
192-
<Avatar
193-
name="avatar name"
194-
size="medium"
195-
renderIcon={CircleUserInstUIIcon}
196-
/>
197-
)
198-
199-
const avatarSvg = container.querySelector('svg')
200-
expect(avatarSvg).toBeInTheDocument()
201-
})
202-
203-
it('should display a Lucide icon with xx-small Avatar size', async () => {
102+
it('should render Lucide icon when passed as JSX element', async () => {
204103
const { container } = render(
205104
<Avatar
206105
name="avatar name"
207-
size="xx-small"
208-
renderIcon={UserInstUIIcon}
106+
size="large"
107+
renderIcon={<HeartInstUIIcon />}
209108
/>
210109
)
211110

212-
const avatarSvg = container.querySelector('svg')
213-
expect(avatarSvg).toBeInTheDocument()
214-
})
215-
216-
it('should display a Lucide icon with x-small Avatar size', async () => {
217-
const { container } = render(
218-
<Avatar name="avatar name" size="x-small" renderIcon={UserInstUIIcon} />
219-
)
220-
221-
const avatarSvg = container.querySelector('svg')
222-
expect(avatarSvg).toBeInTheDocument()
111+
const svg = container.querySelector('svg')
112+
expect(svg).toBeInTheDocument()
223113
})
224114

225-
it('should accept a JSX element and clone it with size/color props', async () => {
226-
const MockIcon = vi.fn((props: any) => (
227-
<svg
228-
data-testid="jsx-icon"
229-
data-size={props.size}
230-
data-color={props.color}
231-
>
232-
<circle cx="25" cy="75" r="20" />
233-
</svg>
234-
))
235-
;(MockIcon as any).displayName = 'wrapLucideIcon(MockIcon)'
236-
237-
const { container } = render(
238-
<Avatar name="avatar name" size="large" renderIcon={<MockIcon />} />
239-
)
240-
241-
const icon = container.querySelector('[data-testid="jsx-icon"]')
242-
expect(icon).toBeInTheDocument()
243-
expect(icon).toHaveAttribute('data-size', 'lg')
244-
expect(icon).toHaveAttribute('data-color')
245-
})
246-
247-
it('should override props when JSX element is passed', async () => {
248-
const MockIcon = (props: any) => (
249-
<svg data-testid="override-icon" data-size={props.size}>
250-
<circle cx="25" cy="75" r="20" />
251-
</svg>
252-
)
253-
MockIcon.displayName = 'wrapLucideIcon(MockIcon)'
254-
115+
it('should render Lucide icon from render function', async () => {
255116
const { container } = render(
256117
<Avatar
257118
name="avatar name"
258-
size="x-large"
259-
renderIcon={<MockIcon size="wrong" />}
260-
/>
261-
)
262-
263-
const icon = container.querySelector('[data-testid="override-icon"]')
264-
expect(icon).toBeInTheDocument()
265-
// Avatar should override the size prop
266-
expect(icon).toHaveAttribute('data-size', 'xl')
267-
})
268-
269-
it('should work with a render function returning JSX', async () => {
270-
const renderFunc = (props: any) => (
271-
<svg data-testid="function-icon" data-size={props.size}>
272-
<circle cx="25" cy="75" r="20" />
273-
</svg>
274-
)
275-
renderFunc.displayName = 'wrapLucideIcon(renderFunc)'
276-
277-
const { container } = render(
278-
<Avatar name="avatar name" size="small" renderIcon={renderFunc} />
279-
)
280-
281-
const icon = container.querySelector('[data-testid="function-icon"]')
282-
expect(icon).toBeInTheDocument()
283-
expect(icon).toHaveAttribute('data-size', 'sm')
284-
})
285-
286-
it('should work with arrow function returning Lucide icon (user pattern)', async () => {
287-
const MockIcon = vi.fn((props: any) => (
288-
<svg data-testid="arrow-lucide-icon" data-size={props.size}>
289-
<circle />
290-
</svg>
291-
))
292-
;(MockIcon as any).displayName = 'wrapLucideIcon(MockIcon)'
293-
294-
const { container } = render(
295-
<Avatar
296-
name="Profile"
297-
size="small"
298-
color="accent2"
299-
renderIcon={() => <MockIcon />}
300-
/>
301-
)
302-
303-
const icon = container.querySelector('[data-testid="arrow-lucide-icon"]')
304-
expect(icon).toBeInTheDocument()
305-
// The icon should have received the correct size prop
306-
expect(icon).toHaveAttribute('data-size', 'sm')
307-
})
308-
309-
it('should apply different sizes to arrow function icons', async () => {
310-
const SmallMockIcon = vi.fn((props: any) => (
311-
<svg data-testid="small-icon" data-size={props.size}>
312-
<circle />
313-
</svg>
314-
))
315-
;(SmallMockIcon as any).displayName = 'wrapLucideIcon(SmallIcon)'
316-
317-
const LargeMockIcon = vi.fn((props: any) => (
318-
<svg data-testid="large-icon" data-size={props.size}>
319-
<circle />
320-
</svg>
321-
))
322-
;(LargeMockIcon as any).displayName = 'wrapLucideIcon(LargeIcon)'
323-
324-
const { container: smallContainer } = render(
325-
<Avatar
326-
name="Profile"
327119
size="small"
328-
renderIcon={() => <SmallMockIcon />}
120+
renderIcon={() => <HeartInstUIIcon />}
329121
/>
330122
)
331123

332-
const { container: largeContainer } = render(
333-
<Avatar
334-
name="Profile"
335-
size="xx-large"
336-
renderIcon={() => <LargeMockIcon />}
337-
/>
338-
)
339-
340-
const smallIcon = smallContainer.querySelector(
341-
'[data-testid="small-icon"]'
342-
)
343-
const largeIcon = largeContainer.querySelector(
344-
'[data-testid="large-icon"]'
345-
)
346-
347-
expect(smallIcon).toBeInTheDocument()
348-
expect(largeIcon).toBeInTheDocument()
349-
350-
// Verify different sizes were passed correctly
351-
expect(smallIcon).toHaveAttribute('data-size', 'sm')
352-
expect(largeIcon).toHaveAttribute('data-size', '2xl')
124+
const svg = container.querySelector('svg')
125+
expect(svg).toBeInTheDocument()
353126
})
354127
})
355128

@@ -365,7 +138,7 @@ describe('<Avatar />', () => {
365138

366139
it('should display the image even if an icon is provided', async () => {
367140
const { container } = render(
368-
<Avatar name="avatar name" src={src} renderIcon={UserInstUIIcon} />
141+
<Avatar name="avatar name" src={src} renderIcon={HeartInstUIIcon} />
369142
)
370143
const avatarImg = container.querySelector('img')
371144
expect(avatarImg).toHaveAttribute('src', src)
@@ -434,7 +207,7 @@ describe('<Avatar />', () => {
434207
name="Jessica Jones"
435208
color="accent2"
436209
hasInverseColor
437-
renderIcon={UserInstUIIcon}
210+
renderIcon={HeartInstUIIcon}
438211
/>
439212
)
440213
const element = container.querySelector('div')

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,26 @@
2323
*/
2424

2525
import { useStyle } from '@instructure/emotion'
26-
import { useState, useEffect, forwardRef, SyntheticEvent } from 'react'
26+
import React, { useState, useEffect, forwardRef, SyntheticEvent } from 'react'
2727

2828
import {
2929
passthroughProps,
30-
renderLucideIconWithProps
30+
IconPropsProvider
3131
} from '@instructure/ui-react-utils'
3232
import { AvatarProps, avatarSizeToIconSize } from './props'
3333

3434
import generateStyle from './styles'
3535

36+
const ICON_COLOR_MAP = {
37+
accent1: 'accentBlueColor',
38+
accent2: 'accentGreenColor',
39+
accent3: 'accentRedColor',
40+
accent4: 'accentOrangeColor',
41+
accent5: 'accentGreyColor',
42+
accent6: 'accentAshColor',
43+
ai: 'onColor'
44+
} as const
45+
3646
/**
3747
---
3848
category: components
@@ -138,13 +148,15 @@ const Avatar = forwardRef<HTMLDivElement, AvatarProps>(
138148
//icon in avatar
139149
if (renderIcon) {
140150
const iconSize = avatarSizeToIconSize[size]
141-
// TODO we should never do this, do not create a fake style
142-
const iconColor = styles?.iconColor
151+
const iconColor = hasInverseColor ? 'onColor' : ICON_COLOR_MAP[color]
143152

144-
return renderLucideIconWithProps(renderIcon, {
145-
size: iconSize,
146-
color: iconColor
147-
})
153+
return (
154+
<IconPropsProvider size={iconSize} color={iconColor}>
155+
{typeof renderIcon === 'function'
156+
? React.createElement(renderIcon as any)
157+
: (renderIcon as React.ReactElement)}
158+
</IconPropsProvider>
159+
)
148160
}
149161

150162
//initials in avatar

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ import type {
3131
} from '@instructure/emotion'
3232
import type {
3333
AsElementType,
34-
OtherHTMLAttributes
34+
OtherHTMLAttributes,
35+
Renderable
3536
} from '@instructure/shared-types'
36-
import { Renderable } from '@instructure/shared-types'
3737

3838
const avatarSizeToIconSize = {
3939
'xx-small': 'xs',
@@ -101,7 +101,7 @@ type AvatarOwnProps = {
101101
* An icon, or function that returns an icon that gets displayed. If the `src` prop is provided, `src` will have priority.
102102
* When using Lucide icons, Avatar will automatically pass the appropriate size and color props based on the Avatar's size and color.
103103
*/
104-
renderIcon?: Renderable<{ size?: string | number; color?: string }>
104+
renderIcon?: Renderable
105105
}
106106

107107
export type AvatarState = {
@@ -116,9 +116,7 @@ type AvatarProps = AvatarOwnProps & {
116116
themeOverride?: ThemeOverrideValue
117117
} & OtherHTMLAttributes<AvatarOwnProps>
118118

119-
type AvatarStyle = ComponentStyle<'avatar' | 'image'> & {
120-
iconColor?: string
121-
}
119+
type AvatarStyle = ComponentStyle<'avatar' | 'image'>
122120

123121
const allowedProps: AllowedPropKeys = [
124122
'name',

0 commit comments

Comments
 (0)