<Avatar/> supports svgs passed to image prop

[r100-stack]

Changes

Avatar's image prop can now accept <svg> children and display it correctly.

Testing

Added a unit test to confirm svgs passed to the image prop is included in the dom. Added a With Icon story and tested the the svg displays properly with the background color.

Docs

Added a changeset

TODOs

• Add/approve visual and unit tests

[mayank99]

Maybe I missed something, but what is the benefit of using <Icon> inside <Avatar> if the size is being overridden anyway? The user should just pass an svg directly.

Also, what is the benefit of the deprecation? The same image prop could be reused.

The explicit iui-icon class also does not need to be supported.

[r100-stack]

Maybe I missed something, but what is the benefit of using <Icon> inside <Avatar> if the size is being overridden anyway? The user should just pass an svg directly.

There's no benefit. I just thought that since we're allowing users to wrap svgs in <Icon>, they might do this all the time. So they might also pass <Icon><SvgAdd/></Icon> to Avatar. So we should catch this case also.

Also, what is the benefit of the deprecation? The same image prop could be reused.

I could reuse the image prop. I thought of changing it to child to make it more clear that svgs (or other elements) can be passed.

The explicit iui-icon class also does not need to be supported.

I included that test since that is how icons are added in avatars in the avatar HTML demo page.

Also, the styling in avatar.scss targets .iui-icon (https://github.com/iTwin/iTwinUI/pull/1040/files#diff-be3c849b1fcfeb4dda01cfe9c31aa0e237ab0e06d71a2838d5ddd77c1700f05aL51-R53)

[mayank99]

There's no benefit. I just thought that since we're allowing users to wrap svgs in <Icon>, they might do this all the time. So they might also pass <Icon><SvgAdd/></Icon> to Avatar. So we should catch this case also.

If there is no benefit, then lets not add the extra complexity. Currently the icon component is coupled to the avatar for no reason.

I could reuse the image prop. I thought of changing it to child to make it more clear that svgs (or other elements) can be passed.

image seems fine to me. svgs are technically still images. But if you want to deprecate it, then you could use children instead of child. it feels more natural in JSX.

The explicit iui-icon class also does not need to be supported.

I included that test since that is how icons are added in avatars in the avatar HTML demo page.

Also, the styling in avatar.scss targets .iui-icon (#1040 (files))

The class names are considered "implementation detail" for React components. It's totally fine to support them in CSS, but React users are never expected to pass the iui-icon class. In the future we could rename this class to iui-avatar-icon or something, and that should not break react users.

[FlyersPh9]

Is this considered a breaking change for our CSS pkg?

[r100-stack]

Ohh, right, it looks like a breaking change. It also breaks the avatar demo html page.

I auto-committed the suggestion (#1040 (comment)), but I think it needs some editing. Will look into it more.

[r100-stack]

Would this be alright? This keeps the html demos the same, is not a breaking change, and also targets svgs without .iui-icon which is likely how icons are passed on the react side.

Suggested change

	svg {
	> .iui-icon, > svg {

[mayank99]

looks like you need to add these three lines to fix the positioning

left: 50%;
top: 50%;
transform: translate(-50%, -50%);

and then the wrapping section would need display: flex; align-items: end

alternatively, display: grid could be used instead of position: absolute to center the svg. I think this would avoid the need to set styles on the wrapper.

i do not think it would be a breaking change because anyone using iui-icon will be unaffected.

[r100-stack]

Thanks, the first option you mentioned worked. Now targeting the avatar's svg instead of > .iui-icon (7a2f509 (#1040))

[mayank99]

Looks good. Just need @FlyersPh9's help to find a new default color (#1040 (comment))

[FlyersPh9]

Good to go once this change is made.