Improvements to the Avatar component #57
Conversation
Deploying with
|
| Latest commit: |
2d454e2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1b193b72.compound-web.pages.dev |
| Branch Preview URL: | https://germain-gg-avatar-changes.compound-web.pages.dev |
| export const Avatar = forwardRef< | ||
| HTMLSpanElement | HTMLButtonElement, |
There was a problem hiding this comment.
Could this please be generic depending on onClick's presence or as so we don't need to do type assertions on the caller side
There was a problem hiding this comment.
I don't think you have access to the ref right here. How would you express that?
There was a problem hiding this comment.
Yeah this gets a lot trickier with forwardRef but I think bad types here will yield landmines and footguns for callers who are unfamiliar with compound.
There was a problem hiding this comment.
I think this expresses the reality of the ref, it can either be a button or a span. I can make it more generic and say HTMLElement if you think that brings more value 🤷
There was a problem hiding this comment.
Can you document it at least onClick ? button : span (but in English)? Is the intention for the storybook to be the developer documentation too? Compound seems to be lacking jsdoc or any documentation for what props mean. Like type=square doesn't say anything about it being a rounded square, onClick doesn't say it'll change the underlying element etc. The overall component doesn't say what its for, that it generates a fallback avatar. I think the lack of documentation puts us in a far worse place than using BaseAvatar in react-sdk.
I think adding JSDoc (and maybe exposing it into Storybook) for intellisense and other IDE tools to pick up would be crucial for making the best (safe) use of the components Compound brings
There was a problem hiding this comment.
Have added JSDoc comments, it was definitely lacking, and I will actually document all the other components with JSDoc comment to make sure this practise spreads across the codebase. Thank you for calling that out.
We would likely not ever document implementation details like "square has rounded corners". But as discussed out of band, documenting the underlying element used, can be greatly beneficial from an a11y point of view.
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
There was a problem hiding this comment.
Ideally we'd use techniques from https://stackoverflow.com/a/58473012 to fix the Ref type to match the underlying html element in use
For matrix-org/matrix-react-sdk#11393 and element-hq/compound#137
Makes the button polymorphic and allows it to be a
buttonas well as a normal Avatar.The image now can load lazily as well.
And we have an improved Storybook story.