Support SVGs as images in notification components #1720
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 12984db The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## main #1720 +/- ##
==========================================
- Coverage 91.33% 91.32% -0.01%
==========================================
Files 170 170
Lines 3624 3644 +20
Branches 1200 1251 +51
==========================================
+ Hits 3310 3328 +18
Misses 296 296
- Partials 18 20 +2
|
packages/circuit-ui/components/NotificationFullscreen/NotificationFullscreen.spec.tsx
Outdated
Show resolved
Hide resolved
| const Svg = image; | ||
| return ( | ||
| <div css={imageStyles}> | ||
| <Svg css={svgStyles} /> |
There was a problem hiding this comment.
One downside of this approach, compared to the ImageProps one, is that there's no required alternative text for the asset. On the other hand, for notification SVGs (at least in the SumUp context), it's likely that the asset will only be illustrative and should have aria-hidden="true".
Were you thinking of leaving this concern to developers?
There was a problem hiding this comment.
Good point! I actually didn't think of that 🙈
Would you prefer an optional alt text prop or simply adding aria-hidden="true"/role="presentation" by default? (I haven't seen any use cases where an alt text would have been necessary)
There was a problem hiding this comment.
Would you prefer an optional alt text prop or simply adding aria-hidden="true"/role="presentation" by default?
My thoughts 👇
Decorative media (by default)
Adding aria-hidden or a presentation role to the SVG is probably the "cleanest" and would work in most (all?) cases today, but I'm not sure we can/should assume that SVGs will always be decorative.
If they're only decorative by default, this might be a good option. What API would you suggest to override the default and add alternative text or a label? The option I'm thinking of is quite verbose.
Also, if we go down that route, I'd also make the img decorative (for consistency).
Alt text
I like this option as well but it might also make the API a bit messy—at least with the option i'm thinking of:
type Props = {
image?: {
src?: string,
children?: FC<SVGProps<SVGSVGElement>>,
alt: string, // must always be provided, even if it's `""` for a decorative image
},
/* etc */
}My thinking:
- This is nice because alternative text is always required, no matter if the actual asset is an
imgor ansvg - ...on the other hand
altisn't appropriate on ansvg, so internally we should check which of the two elements is rendered and either usealtoraria-label - BUT! I really don't like APIs like this where you could pass either an src or children—it makes it confusing. But it would be equally confusing to have something like
src: string | FC<SVGProps<SVGSVGElement>> - ...and finally I don't like how this is moving away from the
ImageProps
Did you have other APIs in mind? Right now I'm not convinced by any solution 😅
There was a problem hiding this comment.
Alright, so you want to have the option to provide alternative text, but by default, the image/illustration should be hidden from screen reader users.
altisn't appropriate on ansvg, so internally we should check which of the two elements is rendered and either usealtoraria-label
Yes. I would stick to alt as the prop name as the aria-label is also "alternative text" and developers are already familiar with its purpose.
I would call the SVG something other than children since that would imply a ReactNode, but we're expecting a ReactElement. So element perhaps? Or svg? 🤔
I don't like how this is moving away from the
ImageProps
Eh, I feel you, but I think there's no better approach to this.
So, we end up with:
type Props = {
image?: {
src: string,
alt?: string, // defaults to `""`
} | {
children: FC<SVGProps<SVGSVGElement>>,
alt?: string, // defaults to `""`
},
/* etc */
}There was a problem hiding this comment.
I'm thinking of going for this:
type Props = {
image: ImageProps | { svg: FC<SVGProps<SVGSVGElement>>, alt: string },
/* etc */
}Reasoning here is that although the image will often be decorative, I'd still like to consistently require the alt prop when we render graphics across Circuit UI, just to make 100% sure that developers think about it. It'll often be alt="" and that's okay 🙂
Also the naming "svg" is a good choice here IMO: we need the element to be an SVG so I think it doesn't make sense to make the naming broader (e.g. "element").
Let me know if you have concerns!
| <div | ||
| class="circuit-1" | ||
| > | ||
| <svg |
There was a problem hiding this comment.
I think this would get announced "image"
There was a problem hiding this comment.
Should be solved with aria-hidden that is passed to an svg element if the alt value is "". Will verify with manual testing
There was a problem hiding this comment.
Chrome x VoiceOver:
| img | svg | |
|---|---|---|
| alt | Screen.Recording.2022-09-09.at.17.48.37.mov |
Screen.Recording.2022-09-09.at.17.54.43.mov |
| !alt | Screen.Recording.2022-09-09.at.17.51.21.mov |
Screen.Recording.2022-09-09.at.17.57.00.mov |
TL;DR: everything looks good with this user agent. I think we can roll with it
…tionFullscreen.spec.tsx Co-authored-by: Robin Métral <robin.metral@sumup.com>
|
FYI I'm picking this up 🙂 Following-up in the existing discussions above |
This also improves tests and type descriptions.
This also improves tests and type descriptions.
|
Thanks 💙 I'll merge and release this on Monday. Thank you for your contributions to this! |
Closes #1707.
Approach and changes
NotificationFullscreenandNotificationModalcomponentsDefinition of done