Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set UserAvatar image size in pixels, not percents #1870

Merged
merged 6 commits into from
Jul 22, 2021

Conversation

goran-peoski-work
Copy link
Contributor

@goran-peoski-work goran-peoski-work commented Jul 21, 2021

Fix issue where <UserAvatar> would display oval (not circular) image due to using % instead of px in SCSS.
Suplemental to #1263

While #1263 tries to adjust for the image source (src prop) itself not being square size or even missing, this PR deals only with the <img> element size not being square when using percents.

BEFORE:
sparkle-user-avatar-04

AFTER:
sparkle-user-avatar-03

@goran-peoski-work goran-peoski-work requested a review from a team as a code owner July 21, 2021 06:49
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 06:49 Inactive
@github-actions github-actions bot added the 💎 styles For (S)CSS style related issues/changes/improvements/etc. label Jul 21, 2021
@goran-peoski-work goran-peoski-work added 💪 enhancement Enhancements/improvements to existing functionality 🐛 bug A bug, error, issue, problem, etc within the platform. labels Jul 21, 2021
@codeclimate
Copy link

codeclimate bot commented Jul 21, 2021

Code Climate has analyzed commit b07a4e5 and detected 0 issues on this pull request.

View more on Code Climate.

@goran-peoski-work goran-peoski-work enabled auto-merge (squash) July 21, 2021 06:51
@github-actions
Copy link

github-actions bot commented Jul 21, 2021

Visit the preview URL for this PR (updated for commit b07a4e5):

https://co-reality-staging--preview-pr-1870-iel0hxin.web.app

(expires Thu, 29 Jul 2021 11:00:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 14:29 Inactive
src/components/atoms/UserAvatar/UserAvatar.tsx Outdated Show resolved Hide resolved
src/components/atoms/UserAvatar/UserAvatar.tsx Outdated Show resolved Hide resolved
@@ -56,8 +56,7 @@ export const _UserAvatar: React.FC<UserAvatarProps> = ({

const containerClasses = classNames("UserAvatar", containerClassName, {
"UserAvatar--clickable": onClick !== undefined,
"UserAvatar--large": large,
"UserAvatar--medium": medium,
[`UserAvatar--${size}`]: size,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Size is limited to a handful values, it shouldn't be a problem either being undefined or "legacy", "small" etc.

@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 21, 2021 15:13 Inactive
@mike-lvov mike-lvov disabled auto-merge July 21, 2021 17:24
@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 22, 2021 10:19 Inactive
Copy link
Contributor

@mike-lvov mike-lvov left a comment

Choose a reason for hiding this comment

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

LGTM

&--#{$modifier},
&--#{$modifier} img {
@include square-size($size);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The syntax is shorter but the readability is not better. We should choose one and keep it consistent, I haven't seen this syntax in the project so far. And we need to communicate these things before applying them. If I start adding my own standards and styles with every PR you guys will hate me by the end of the week 😆

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 haven't seen this syntax in the project so far.

https://github.com/sparkletown/sparkle/blob/staging/src/scss/constants.scss contains mixins, functions, interpollation...

Standard SCSS syntax really https://sass-lang.com/documentation
and my view is if we don't use it as intended to make few things DRY and robust, might as well move to back to CSS.

And we need to communicate these things before applying them.

PR review process is communication, PR merge is application, so we're good I think.

you guys will hate me by the end of the week

Not really. Not a fan of setting things in stone.

@goran-peoski-work goran-peoski-work temporarily deployed to feature-preview July 22, 2021 10:57 Inactive
@goran-peoski-work goran-peoski-work merged commit 0c363cc into staging Jul 22, 2021
@goran-peoski-work goran-peoski-work deleted the fix/user-avatar-image-shape branch July 22, 2021 11:03
@sabrinaorban
Copy link
Contributor

My safe formula for the avatar sizes would be:

  • for avatars up to 55px, the indicator should be 28% of the avatar size, and the indicator border should be 12% of the indicator size
  • for avatars bigger than 55px, if it’s ever the case we are safe with a 16% indicator size
  • I don’t know all the cases where we use all these avatar sizes to check how it looks on the platform but I tested it on a design file and it seems to work alright
  • the indicator border is extra, it makes the indicator stand out in an elegant way. it would be nice if the border could have the color of the background behind the avatar

Screenshot 2021-07-22 at 18 16 39

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🐛 bug A bug, error, issue, problem, etc within the platform. 💪 enhancement Enhancements/improvements to existing functionality 💎 styles For (S)CSS style related issues/changes/improvements/etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants