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

feat(organizations): Add isEmpty to avatar component and use it for members table TASK-1420 #5457

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Jan 28, 2025

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📣 Summary

Ghost variant was added to display a "ghost" state in members table when user is invited but still not active

📖 Description

  • Added a isEmpty prop for our existent Avatar component for it to render the empty-dashed circle.
  • At first I intended to add theme and use Mantine's Avatar component, but since our avatar has so many specific details the effort wouldn't be worth it, so I just customized the existent component.
  • In the future we can migrate the internal structure of our avatar component to use Mantine's layout components.
  • Avatar story was changed to add the isEmpty as a config option

👀 Preview steps

One can check the implementation result in Avatar's storybook story
For reference, I'm adding a screenshot of the property in use:
image

@pauloamorimbr pauloamorimbr marked this pull request as ready for review January 28, 2025 21:24
</div>
{isGhost ? (
<div className={cx(styles.initials, styles.ghost)}>
<svg width='100%' viewBox='0 0 24 24'>
Copy link
Contributor Author

@pauloamorimbr pauloamorimbr Jan 28, 2025

Choose a reason for hiding this comment

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

Decided to use SVG so we could have control over the dash offset and size

Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

Left some comments for your consideration, but not blocking

@pauloamorimbr pauloamorimbr changed the title feat(organizations): Add ghost variant to avatar for members table TASK-1420 feat(organizations): Add isEmpty to avatar component and use it for members table TASK-1420 Jan 30, 2025
@pauloamorimbr
Copy link
Contributor Author

Thanks for the inputs, @jamesrkiger. Did some changes to adjust based on them.

@pauloamorimbr pauloamorimbr self-assigned this Jan 30, 2025
Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

Sorry, I've got one last issue for you:

If you use storybook and toggle the isEmpty control on a small avatar component, you will notice the component's alignment shifting (by a very small amount, but visible nonetheless). It looks like the size of the circle changes. It's not noticeable when the size is 'm', but the size changes there, too. It's like a 1 pixel difference, but still probably better to just fix it now rather than later.

Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

lgtm

@pauloamorimbr pauloamorimbr merged commit f1e67c7 into main Jan 30, 2025
7 checks passed
@pauloamorimbr pauloamorimbr deleted the pamorim/task-1420-add-ghost-variant-to-avatar branch January 30, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants