feat(UserAvatar): implementation of name,size props

[anamikaanu96]

Contributes to #4017

Implementation of name and size props to useravatar

What did you change?

packages/ibm-products/src/components/UserAvatar/UserAvatar.js
packages/ibm-products/src/components/UserAvatar/UserAvatar.stories.js
packages/ibm-products/src/components/UserAvatar/UserAvatar.test.js
packages/ibm-products-styles/src/components/UserAvatar/_user-avatar.scss

How did you test and verify your work?

storybook

[netlify]

✅ Deploy Preview for carbon-for-ibm-products ready!

Name	Link
🔨 Latest commit	bd86fa7
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-for-ibm-products/deploys/65d2fa13e37ec400082eb442
😎 Deploy Preview	https://deploy-preview-4312--carbon-for-ibm-products.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[elycheea]

I think we can move size to defaults.

I missed this in the previous PRs, but we probably should not set a default for tooltipText since this is optional. The default here should only be set in the story.

Suggested change

	size = 'md',
	tooltipText = defaults.tooltipText,
	size = 'md',
	tooltipText = defaults.tooltipText,

[anamikaanu96]

Removed default from useravatar.js file, added default size in story

[elycheea]

The corresponding icon size should always be the same based on the avatar size.
Rather than checking for renderIcon, I think it makes more sense to just render the icon¹ that gets passed in using the sizes mapped below.

Size	Icon scale
xl	32
lg	24
md	20
sm	16

Footnotes

1. I think we have a few examples in ButtonMenu, ComboButton, EditInPlace/InlineEdit and a few other places like if you’d like to see some examples although they don’t include size. ↩

[anamikaanu96]

Changed based on @davidmenendez suggestion

[davidmenendez]

good start! i think we can make some small adjustments to make this code a little more clean

[davidmenendez]

i think this needs to be changed. currently there's no way to access the group variant without already passing the group icon, which we then import in this component. given that i think the component needs to be refactored slightly.

the user can just pass in an icon directly. <UserAvatar renderIcon={Information} size="md" />

in the render we can check for a custom icon or provide a default one. and then we can use a size dictionary to access the sizes.

size,
renderIcon: Icon,
...
const sizes = {
  sm: 16,
  md: 20
}
...
{Icon ? <Icon size={sizes[size]} /> : <User />

[anamikaanu96]

Done

[davidmenendez]

making good progress! just a few more suggestions. i'd also like to suggest doing something like this to clean up the render portion. instead of SetItem i'd do something like this

const Avatar = () => (
  <div
    {...rest}
    className={cx(
      blockClass,
      className,
      `${blockClass}--${backgroundColor}`,
      `${blockClass}--${size}`
    )}
    ref={ref}
    role="img"
    {...getDevtoolsProps(componentName)}
  >
    {getItem()}
  </div>
);

if (tooltipText) {
  return (
    <Tooltip
      align={tooltipAlignment}
      label={tooltipText}
      className={`${blockClass}__tooltip ${carbonPrefix}--icon-tooltip`}
    >
      <TooltipTrigger>
        <Avatar />
      </TooltipTrigger>
    </Tooltip>
  );
}

return <Avatar />;

[davidmenendez]

we should still have a defaults constant and use a default value here. probably md

[davidmenendez]

since the user isn't passing props directly to the icon this way we can reference this as a component to be consumed directly. i'll follow this up in an additional comment renderIcon: RenderIcon

[davidmenendez]

this line isn't necessary when we declare renderIcon as a component in the props. and since, as i mentioned before, the user isn't passing in any props directly to icon we don't need to return a function. we can just do something like this

renderIcon: RenderIcon,
...
const getItem = () => {
  const iconProps = {
    size: iconSize[size],
  };
  if (RenderIcon) {
    return <RenderIcon {...iconProps} />;
  }
  if (name) {
    return formatInitials;
  }
  return <User {...iconProps} />;
};

[elycheea]

Also note the follow up issues #4332 — after seeing demo today, think we should use initials as default and only render an icon if passed in.

[davidmenendez]

makes things even easier!

[davidmenendez]

just noticed that we should also tweak this a bit. two problems here

1. when a 2 character string is passed in we're not formatting it with toUpperCase
2. if a single string is passed in that's greater than 2, like David for example, the output is DA which i wouldn't expect because no last name was passed in.

i think we can address this and improve the readability by making some changes

const formatInitials = () => {
  const parts = name.split(' ');
  const firstChar = parts[0].charAt(0).toUpperCase();
  if (parts.length === 1) {
    return firstChar;
  }
  const lastChar = parts[parts.length - 1].charAt(0).toUpperCase();
  const initials = [firstChar];
  if (lastChar) {
    initials.push(lastChar);
  }
  return ''.concat(...initials);
};

this implementation is slightly more readable than the regex and it clearly demonstrates each outcome

[elycheea]

Agree with toUpperCase but didn’t mind DA if only two letters are passed. I feel like I’ve seen similar approaches before but unsure if this was intended behavior in our case. 🤔

[davidmenendez]

i'll open up an additional issue to confirm this behavior 👍

[elycheea]

@davidmenendez I think this was actually determined and set in the original behavior of user profile image/initials.

I guess the original Regex does accomplish this as well. I’m all for readability, but I think either can work in this case.

[elycheea]

Also found some of the initial discussion (ha ha) that I believe ties to the original implementation. #69 (comment)

It looks like this is to better support use of display names where maybe there is not a first and last name provided or used. Again, probably mostly just a fallback, but I do think we can probably respect the original implementation of this.

(Happy with either the Regex or refactored version of this.)

[anamikaanu96]

sticked with the refactored version.

[davidmenendez]

awesome!

[davidmenendez]

@elycheea i don't see anything specifically mentioned in the documentation regarding only first names. this takes multiple last names in account, but not just a single one. seems like a funny oversight. good thing Cher isn't a user 😂

i think this PR is good to go and i'll go ahead and open another issue to verify the single name question 👍

[anamikaanu96]

Done all the required changes, @elycheea could you please recheck

[elycheea]

Approving although I think we still have a few fixes to make with formatInitials().

If you prefer to merge this as is and open a separate PR, feel free to merge this now, and we can open a separate issue to update this specifically.

Even if we keep the refactored version, it should support these scenarios:

If passed the following as name

1. Thomas J. Watson becomes TW
2. Thomas Watson becomes TW
3. thomasjwatson becomes TH

In the last scenario, we have support in case a name (or display name) is passed that does not follow the traditional First Name, Last Name.

(See UserProfileImage)