-
Notifications
You must be signed in to change notification settings - Fork 224
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
fix(avatar): Combine Avatar & AvatarButton and provide fallback image #614
Conversation
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Overall, I like the idea. A few questions on the implementation to make it a better experience. |
@anicholls @NicholasBoll Updated and unified as a single functional component, let me know what you think. |
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.
Can you rebase this onto prerelease/v4
and change the base branch as well?
Also, can you summarize the breaking changes in the PR description?
# Conflicts: # modules/avatar/react/spec/AvatarButton.spec.tsx
Co-authored-by: Alex Nicholls <anicholls3@gmail.com>
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.
Looks great - thank you! 🎉
modules/avatar/react/lib/Avatar.tsx
Outdated
/** | ||
* The ref to the button or div that the styled component renders. | ||
*/ | ||
ref?: React.Ref<HTMLButtonElement>; |
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 don't think this will work. Let me test it out.
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 considered adding the ref in the Avatar overload but wasn't sure if it was needed since in Alex's example it wasn't split up.
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 have confirmed this will not work and React will throw the following warning:
react.development.js:167 Warning: Avatar: `ref` is not a prop. Trying to access it will result in `undefined` being returned. If you need to access the same value within the child component, you should pass it as a different prop.
I think ref
is the correct prop, but we'll have to use forwardRef
for the component to make that work. Some more details here: #548
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 catch 👍 We should make sure we have a test for this as well.
Summary
If the link for the avatar fails, eg 404 etc, we are currently showing a broken link. This could be made a bit nicer by rendering the default icon instead.
Not sure the best way to test this, should I just add an item to the storybook for avatar image?
Also open question if the icon should have a label with the altText passed in on failure?
Closes #613
BREAKING CHANGES
<AvatarButton>
has been removed. By default<Avatar>
will now be a button. If you need the old plain div version you can pass the propas="div"
.The component is now a functional component instead of a class. If you are using ref on the class version it will not be pointing to the same thing.
buttonRef
has changed toref
since it could now reference a button or a divVisual change: Avatar images appear once they are load. While loading or if they fail to load the default icon will be shown. So you may want to check which variant you are using even in the image case.
Checklist
yarn test
passesExample