fix(InlineNotification): make statusIconDescription optional

[asudoh]

Given statucIconDescription in <InlineNotification> and one in <ToastNotification> have a fallback value, they shouldn't have isRequired in their prop types. This change fixes that.

Fixes #5527.

Changelog

Changed

• Make statusIconDescription prop optional.

Testing / Reviewing

Testing should make sure <InlineNotification> and <ToastNotification> are not broken

[netlify]

Deploy preview for carbon-elements ready!

Built with commit ef11fd8

https://deploy-preview-5528--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit ef11fd8

https://deploy-preview-5528--carbon-components-react.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit c957756

https://deploy-preview-5528--carbon-components-react.netlify.com

[netlify]

Deploy preview for carbon-elements ready!

Built with commit c957756

https://deploy-preview-5528--carbon-elements.netlify.com

[emyarod]

looks good to me

[joshblack]

Would we prefer to not include the fallback value? It seems to not be adding much value and seemingly would read better if we had people specify a value. Also would understand if we wanted to improve the fallback value and make this change? Down either way.

[asudoh]

@tw15egan Any thoughts on the fallback value? Thanks!

[tw15egan]

We should be consistent with other SVG description props throughout our components, so if we do not normally require them / provide fallback values, then they should be removed

[asudoh]

Good point @tw15egan, one question on this front. Though we have a fallback value, I think most application needs to provide statusIconDescription anyway for the sake of g11n. That said, would we consider this property effectively required/optional?

[tw15egan]

If we're going to keep it required, then yeah we should remove the fallback since they will need to specify their own to satisfy g11n requirements. However, it seems like iconDescription has a defaultProp of 'closes notification'. So isn't that essentially doing the same thing?

This seems to be another example of prop inconsistency that should be a major focus for v11. If we are requiring a prop, should we provide fallbacks that get picked up in testing, or make the user handle these cases right away by having a console warning?

The initial prop was just added because it was initially just getting the same text as the close button.

So I guess there are two options:

1. Remove the required, since it has a fallback even if it is not specified. It would expect teams to handle this case during g11n testing, otherwise, they get the default English string.

2. Keep it required, but remove the fallback. This would immediately alert users that they need to handle g11n for this prop on their own but would be pretty noisy. If we go this route, I'd probably add it to the list of things we want to do for v11 around prop consistency in our components.

[asudoh]

Good question, here's what I understand; The fallback value like one for iconDescription is for providing good default that allows quick demonstration, etc. But for real-world application, iconDescripiton should be provided as we discussed for the sake of g11n.

For React isRequired thing, IIUC it has to be removed as long as there is a default value. So if we still keep the default (fallback) value there we should remove isRequired. Another option is removing default value there and make them isRequired, but it's a breaking change (at least for iconDescription) as @tw15egan alluded to.

[asudoh]

Talked offline with @tw15egan and removed .isRequired from iconDescription. Thanks @tw15egan!

[tw15egan]

👍 ✅