-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(UserAvatar): implementation of name,size props #4312
feat(UserAvatar): implementation of name,size props #4312
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size = 'md', | ||
tooltipText = defaults.tooltipText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
size = 'md', | |
tooltipText = defaults.tooltipText, | |
size = 'md', | |
tooltipText = defaults.tooltipText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed default from useravatar.js file, added default size in story
const getItem = (renderIcon) => { | ||
if (renderIcon === User) { | ||
return icons.user['md']; | ||
return icons.user[size]; | ||
} else if (renderIcon === Group) { | ||
return icons.group['md']; | ||
return icons.group[size]; | ||
} else if (name) { | ||
return formatInitials; | ||
} else { | ||
return renderIcon; | ||
return icons['user'][size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 icon1 that gets passed in using the sizes mapped below.
Size | Icon scale |
---|---|
xl |
32 |
lg |
24 |
md |
20 |
sm |
16 |
Footnotes
-
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. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed based on @davidmenendez suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good start! i think we can make some small adjustments to make this code a little more clean
@@ -58,8 +58,10 @@ export let UserAvatar = React.forwardRef( | |||
// The component props, in alphabetical order (for consistency). | |||
backgroundColor, | |||
className, | |||
name, | |||
/* TODO: add other props for UserAvatar, with default values if needed */ | |||
renderIcon = defaults.renderIcon, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 />;
tooltipText = defaults.tooltipText, | ||
tooltipAlignment = defaults.tooltipAlignment, | ||
renderIcon, | ||
size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should still have a defaults constant and use a default value here. probably md
renderIcon = defaults.renderIcon, | ||
tooltipText = defaults.tooltipText, | ||
tooltipAlignment = defaults.tooltipAlignment, | ||
renderIcon, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
return icons.user['md']; | ||
} else if (renderIcon === Group) { | ||
return icons.group['md']; | ||
let Iconcomponent = renderIcon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} />;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes things even easier!
xl: 32, | ||
}; | ||
const formatInitials = () => { | ||
if (name.length === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noticed that we should also tweak this a bit. two problems here
- when a 2 character string is passed in we're not formatting it with
toUpperCase
- if a single string is passed in that's greater than 2, like
David
for example, the output isDA
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll open up an additional issue to confirm this behavior 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sticked with the refactored version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
@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 👍 |
Done all the required changes, @elycheea could you please recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thomas J. Watson
becomesTW
Thomas Watson
becomesTW
thomasjwatson
becomesTH
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)
a4e1a21
…stem#4312) * feat(useravatar):implementation of name,size props * fix(useravatar): changed story name * fix(useravatar): review changes suggested * fix(useravatar): resolve test-c4p * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): name changes suggested --------- Co-authored-by: elysia <[email protected]>
…stem#4312) * feat(useravatar):implementation of name,size props * fix(useravatar): changed story name * fix(useravatar): review changes suggested * fix(useravatar): resolve test-c4p * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): name changes suggested --------- Co-authored-by: elysia <[email protected]>
* feat: new component decorator * fix: code and styling updates * fix: udpate snapshots * fix: update gallery * fix: skip some tests (temp) * fix: minor changes * fix: updated PropTypes * fix: updated tests * fix: update styling * fix: updated snapshots and galleries * fix: storybook update * fix: update code, style, docs * fix: design review update * fix: design review update * fix: added a11y icon borders * fix: update icons and colors * fix: tests * fix: docs * fix: styling updates * fix: adds aria label to datagrid toolbar (#4348) * fix: add aria label to datagrid toolbar * chore: format * fix: prop name change * fix(Dataspreadsheet): Reduce duplication with isHoldingCommandKey #4188 (#4227) * feat(UserAvatar): implementation of name,size props (#4312) * feat(useravatar):implementation of name,size props * fix(useravatar): changed story name * fix(useravatar): review changes suggested * fix(useravatar): resolve test-c4p * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): name changes suggested --------- Co-authored-by: elysia <[email protected]> * feat: Tearsheet ai (#4215) * chore: wip * feat: tearsheet AI enhancements * feat: add gradient glow to container * chore(release): publish [skip ci] - [email protected] - @carbon/[email protected] - @carbon/[email protected] - @carbon/[email protected] - @carbon/[email protected] * chore: add new example gallery action (#4352) * chore: add new example gallery action * chore: remove gallery check from CI * chore: remove unused steps 🙃 * feat: Modal ai updates (#4362) * chore: wip * feat: tearsheet AI enhancements * feat: add gradient glow to container * chore: bump carbon versions * feat: modal ai v2 updates * fix: modal body * feat: modal ai v2 updates * Merge branch 'main' into modalAIUpdates * chore: update gallery config * fix(DataGrid): add support for selectrow and nestedrow (#4354) Co-authored-by: Matt Gallo <[email protected]> * feat(Datagrid): add support for multi select filter type (#4361) * chore: custom filter exploration continued * fix(DataGrid): add support for selectrow and nestedrow * chore: custom filter exploration * feat(Datagrid): add multi select option * chore: remove onClearFilters from prop types * chore: revert copyright year * chore: add multi select example to flyout * chore: revert nested/selection changes from branch * chore: update gallery config --------- Co-authored-by: Ratheesh Rajan <[email protected]> * feat(Datagrid): apply latest ai gradients to Datagrid (#4377) * fix(DataGrid): add support for selectrow and nestedrow * feat(Datagrid): apply latest ai gradients * chore: remove comments * chore: revert nested row changes pulled in accidentally * chore: update snapshot * chore: refactor how row class names are applied * fix: remove extra border --------- Co-authored-by: Ratheesh Rajan <[email protected]> * feat(useravatar): image implementation (#4355) * feat(useravatar):implementation of name,size props * fix(useravatar): changed story name * fix(useravatar): review changes suggested * fix(useravatar): resolve test-c4p * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): review changes suggested * fix(useravatar): name changes suggested * feat(useravatar): image implementation * fix(useravatar): resolve lint issue * fix(useravatar): resolve check-gallery * fix(useravatar): review changes suggested --------- Co-authored-by: elysia <[email protected]> * feat: sort icon appear in the Actions column on Datagrid #4339 (#4346) * chore(release): publish [skip ci] - @carbon/[email protected] - @carbon/[email protected] - @carbon/[email protected] - @carbon/[email protected] * fix: move ai gradient to content (#4381) * chore(release): publish [skip ci] - @carbon/[email protected] - @carbon/[email protected] - @carbon/[email protected] * fix: merge conflicts * fix: update publish config in community package to public (#4383) * fix: header border issue introduced with ai slug (#4385) * chore(deps): bump ip to 2.0.1 (#4386) * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/UserAvatar Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/WebTerminal Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/TruncatedList Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/prefix-example Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip from 2.0.0 to 2.0.1 Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(APIKeyModal): add typescript types (#4336) * feat: beginning ts changes for APIKeyModal * chore: add custom typings to address broken types from carbon/react * fix: address remaining type issues * chore: refactor pw input import * chore: refactor customStep types * chore(deps): bump ip to 2.0.1 (#4401) * build(deps): bump ip in /examples/carbon-for-ibm-products/ProductiveCard Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/StatusIcon Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/SidePanel Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/TagSet Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/RemoveModal Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/PageHeader Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/Saving Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/Tearsheet Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps): bump ip in /examples/carbon-for-ibm-products/SearchBar Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: new component status indicator (#4419) * fix: udpate gallery * fix: code review updates * fix: code review updates * chore: update chromedriver resolutions (#4422) * chore: add chromedriver to resolutions Fixes an issue with changed CDN * chore: remove from resolutions Keep latest chromedriver in yarn.lock * fix: tearsheet overlay (#4421) * fix: tearsheet overlay * fix: selector for old overlay --------- Co-authored-by: Matt Gallo <[email protected]> * fix: side panel slug and close position (#4426) * feat(Datagrid): add option to specify an initial sortable column (#4423) * feat(Datagrid): apply initial column sort functionality * fix: return if not using sortable table --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: David Menendez <[email protected]> Co-authored-by: Amal K Joy <[email protected]> Co-authored-by: Anamika T S <[email protected]> Co-authored-by: elysia <[email protected]> Co-authored-by: Lee Chase <[email protected]> Co-authored-by: github-merge-queue <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ratheesh Rajan <[email protected]> Co-authored-by: Matt Gallo <[email protected]> Co-authored-by: SeonyuK <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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