Skip to content
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

Improvements to the Avatar component #57

Merged
merged 9 commits into from
Aug 10, 2023
Merged

Conversation

germain-gg
Copy link
Contributor

For matrix-org/matrix-react-sdk#11393 and element-hq/compound#137

Makes the button polymorphic and allows it to be a button as well as a normal Avatar.
The image now can load lazily as well.

And we have an improved Storybook story.

@germain-gg germain-gg requested a review from nadonomy August 10, 2023 06:58
@germain-gg germain-gg requested a review from a team as a code owner August 10, 2023 06:58
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 10, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

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

View logs

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested the stories 👍

src/components/Avatar/Avatar.module.css Outdated Show resolved Hide resolved
Comment on lines +38 to +39
export const Avatar = forwardRef<
HTMLSpanElement | HTMLButtonElement,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you have access to the ref right here. How would you express that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Member

@t3chguy t3chguy Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
Copy link

@nadonomy nadonomy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new storybook story is super useful, thx!

@germain-gg germain-gg requested a review from t3chguy August 10, 2023 09:27
@germain-gg germain-gg requested a review from t3chguy August 10, 2023 09:38
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs better developer-facing documentation for props & overall component usage & behaviours

@germain-gg germain-gg requested a review from t3chguy August 10, 2023 10:17
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
germain-gg and others added 2 commits August 10, 2023 11:20
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
@germain-gg germain-gg requested a review from t3chguy August 10, 2023 10:45
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd use techniques from https://stackoverflow.com/a/58473012 to fix the Ref type to match the underlying html element in use

@germain-gg germain-gg merged commit 0b231c1 into main Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants