Review fixes for StatusIcon

[dcwarwick]

What will this achieve?

To complete the review of StatusIcon:

• [x] Add DocGen comments for the props.
• [x] Rename the type prop to kind to follow carbon current convention.
• [ ] Add defaults (size='medium', theme='light') for size and theme, and make them non-required (leave type as required), for consistency with carbon's approach. In previous discussion it was thought preferable to make these required to protect against lazy developers :-)
• [x] Add a iconDescription prop, string type, required, and pass it to the icon as a <title> child, exactly as per carbon-react NotificationIcon component (Notification.js line 227).
• [x] Use forwardRef to forward a ref to the icon.
• [x] Put ...rest before other props on the div (i.e. <div {...rest} className={classNames}>.
• [ ] The IconComponent may need to be wrapped in a useEffect to ensure it reacts correctly to changes in size, theme, type.
• [x] blockClass (in js and scss) should be ${pkg.prefix}--status-icon (two hyphens before block name).
• [x] Add a class to the IconComponent, something like ${blockClass}__icon.
• [x] Use the class on the IconComponent svg to replace use of the element type svg in our SCSS. Also, eliminate nesting in the SCSS per annoying carbon rule.
• [x] Include blockClass itself as a class on the main div.
• [x] Add tests for all the props (size, kind, theme, iconDescription) to check they produce the intended output.
• [x] Add a codesandbox for StatusIcon (https://codesandbox.io/dashboard/all/?workspace=7d09c446-5d37-4748-92d5-3d510c6572d4)
• [x] Use 'sm', 'md', 'lg' and 'xl' as values for size for consistency with Carbon.
• [x] Is the containing <div> really necessary? Let's see if we could merge ...rest and the block classes directly onto the IconComponent and eliminate the wrapping <div>.
• [x] Add tests for ...rest and ref being passed through, as per ExampleComponent.
• [x] Make sure StatusIcon is in index.js, _index.scss and package-settings.js as per updated code guidelines.

[sydrosa]

Hi @dcwarwick I'm cc'ing @andrea-gm since we had this conversation re: #3 : add defaults for size and theme since we don't want a light/medium sized icon to render if someone adds a type. We want to protect against lazy developers who wouldn't change the size or theme if they see something rendered.

[dcwarwick]

@sydrosa That seems quite reasonable. It was only a "carbon tends to provide defaults rather than make things required" thought, so if we've had a conversation and feel it's better for this component to make them required then that makes good sense to me. @lee-chase fyi.

[lee-chase]

@sydrosa as functional component always render (unless you use React.memo) I think you can simplify the component to

export const StatusIcon = ({ type, theme, size, className, ...rest }) => {
  const IconComponent = icons[type]?.[size];

  const classNames = cx(className, `${blockClass}--${theme}`, {
    [`${blockClass}--${theme}-${type}`]: type,
  });

  return (
    <div className={classNames} {...rest}>
      {IconComponent && <IconComponent />}
    </div>
  );
};

[dcwarwick]

@sydrosa There's another small issue we missed when looking through this, but which came up when we were looking at Tearsheet: the Carbon values for size are 'sm', 'md', 'lg', 'xl', rather than 'small', 'medium', 'large', 'x-large'. We don't particularly like the shortened versions, but we should align with carbon if possible :-/ I'll add this to the checklist above.

[dcwarwick]

@sydrosa Thanks Syd -- out of interest, why did you change the name of iconDesription to iconLabel?

[sydrosa]

@dcwarwick excellent question. I felt that iconDescription was a misleading prop name since we do not want a visual description of the icon. Instead, for accessibility purposes, we want a string that explains the icon's purpose, or what it should signify to the user. I felt that iconLabel as well as some updated documentation would push dev's in the right direction of what string they should actually provide as a prop.

Additionally, not all teams using the StatusIcon have them mean the same thing -- wsdesign might use the pending icon to mean "time" or something. Having the iconLabel prop allows a dev to differentiate the meaning

[dcwarwick]

Yeah, we do kind of agree: 'description' is not a very useful word here. However, we had a bit of a discussion about this, and looked through Carbon, and they are consistently using iconDescription, so I'm afraid we should stick with being consistent with that. So the bad news is (sorry) can we pls change the name of the prop iconLabel back to iconDescription, despite the fact we don't like the words very much :-) The good news is that everything else is ticketyboo (that's a technical term) so in the same PR as you pop that prop name back you can do the release steps for StatusIcon 🎉 which are:

• the flags for the components in package-settings.js should be changed to true.
• the component SCSS should be included in /src/components/_index-released-only.scss.
• run the tests, recreating snapshots (using yarn jest -u), and check in the updated public CSS snapshot as part of the PR.

[sydrosa]

Yeah, we do kind of agree: 'description' is not a very useful word here. However, we had a bit of a discussion about this, and looked through Carbon, and they are consistently using iconDescription, so I'm afraid we should stick with being consistent with that. So the bad news is (sorry) can we pls change the name of the prop iconLabel back to iconDescription, despite the fact we don't like the words very much :-) The good news is that everything else is ticketyboo (that's a technical term) so in the same PR as you pop that prop name back you can do the release steps for StatusIcon 🎉 which are:

• the flags for the components in package-settings.js should be changed to true.
• the component SCSS should be included in /src/components/_index-released-only.scss.
• run the tests, recreating snapshots (using yarn jest -u), and check in the updated public CSS snapshot as part of the PR.

Hey @dcwarwick - I strongly disagree but happy to make the changes since I know we want to get this component out. Here's why:

• Carbon isn't consistent with their usage of iconDescription. In some places, it's used as an assistive text div(Icon only button), in others, as a <title> element surrounding the SVG and not in the SVG
• Some of Carbon's examples for iconDescription props are wrong. In the ToastNotifcation component, both the iconDescription and <title> element describe the icon instead of it's purpose, which is bad usage.
• The now deprecated Icon component in Carbon uses a prop called iconTitle (which I prefer and think is more favorable to iconLabel) to specify the text that will be read by a screen reader. I liked this approach.

All this to say, this could be part of a larger conversation, and I'm curious your thoughts for when we should break outside of some Carbon standards if we know that it will better serve our user.

[dcwarwick]

@lee-chase FYI.

i agree that if there are cases where we don't like the Carbon pattern and think it might be better to do something different we should open that up as a larger conversation so that we can be confident we can articulate why we've done things differently, and maybe suggest changes up to Carbon in turn.

The Carbon "description" is sometimes non-displayed text in a DIV, sometimes an aria-label, and sometimes as <title> inside an SVG. They've stated that the preferred approach is aria-label, but then they hardly ever actually do that! I don't actually know of any reason to prefer aria-label vs <title> inside the SVG from an accessibility standpoint -- I think they are equivalent.

Carbon uses iconDescription for the <title> inside an SVG icon in things like Filename, Notification, ProgressIndicator, etc, and also has an expandIconDescription on the TableExpandHeader. We also have several props that are following this pattern, such as closeIconDescription on the Tearsheet, settingsIconDescription on the NotificationsPanel, etc. All of these apply the prop as a <title> inside the icon SVG.

As Lee pointed out, if we are aiming to be consistent with Carbon that at least enables us not to have to think too hard about some of these things! Carbon has set a pattern, and we follow it. If Carbon isn't very consistent about it, we can at least aim to be more so.