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
Merged

Set UserAvatar image size in pixels, not percents #1870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a47991
Set UserAvatar image size in pixels, not percents
goran-peoski-work Jul 21, 2021
b40bc48
Rework UserAvatar size prop
goran-peoski-work Jul 21, 2021
9501872
Merge branch 'staging' into fix/user-avatar-image-shape
goran-peoski-work Jul 21, 2021
76220e8
Remove 'legacy' value from UserAvatar size prop
goran-peoski-work Jul 22, 2021
05bd4d0
Merge branch 'staging' into fix/user-avatar-image-shape
goran-peoski-work Jul 22, 2021
b07a4e5
Merge branch 'staging' into fix/user-avatar-image-shape
goran-peoski-work Jul 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 33 additions & 26 deletions src/components/atoms/UserAvatar/UserAvatar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,26 @@
@import "scss/constants";

.UserAvatar {
$avatar-sizes-map: (
small: 25px,
medium: 40px,
large: 54px,
full: 100%,
);
$indicator-sizes-map: (
small: 8px,
medium: 10px,
large: 12px,
full: 25%,
);

// NOTE: parent and img mismatch due to legacy reasons
$default-avatar-size: 25px;
$default-image-size: 100%;
$default-indicator-size: 8px;

@include square-size($default-avatar-size);
position: relative;
width: 25px;
height: 25px;

&:hover {
.UserAvatar__nametag--hover {
Expand All @@ -19,25 +36,24 @@
}

&__image {
height: 100%;
width: 100%;
@include square-size($default-image-size);
border-radius: 50%;
vertical-align: unset;
}

&__status-indicator {
position: absolute;
bottom: -1px;
right: -1px;
bottom: 0;
right: 0;
border-radius: 50%;
height: 8px;
width: 8px;
z-index: z(user-avatar-status-indicator);
display: block;
@include square-size($default-indicator-size);

&--large {
height: 12px;
width: 12px;
@each $modifier, $size in $indicator-sizes-map {
&--#{$modifier} {
@include square-size($size);
}
}
}

Expand All @@ -49,19 +65,11 @@
}
}

&--small {
width: 25px;
height: 25px;
}

&--medium {
height: 40px;
width: 40px;
}

&--large {
height: 54px;
width: 54px;
@each $modifier, $size in $avatar-sizes-map {
&--#{$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.

}

&__nametag {
Expand All @@ -74,12 +82,11 @@

text-align: center;

margin: 0 auto;
margin: 0 -50% 0 auto;
padding: 0 $spacing--xs;

bottom: 10%;
left: 50%;
margin-right: -50%;

border-radius: $border-radius--md;

Expand Down
13 changes: 6 additions & 7 deletions src/components/atoms/UserAvatar/UserAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@ import { useVenueId } from "hooks/useVenueId";

import "./UserAvatar.scss";

export type UserAvatarSize = "small" | "medium" | "large" | "full";

export interface UserAvatarProps {
user?: WithId<User>;
containerClassName?: string;
imageClassName?: string;
showNametag?: UsernameVisibility;
showStatus?: boolean;
onClick?: () => void;
large?: boolean;
medium?: boolean;
size?: UserAvatarSize;
}

// @debt the UserProfilePicture component serves a very similar purpose to this, we should unify them as much as possible
Expand All @@ -33,8 +34,7 @@ export const _UserAvatar: React.FC<UserAvatarProps> = ({
showNametag,
onClick,
showStatus,
large,
medium,
size,
}) => {
const venueId = useVenueId();

Expand All @@ -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.

});

const isOnline = useMemo(
Expand All @@ -76,7 +75,7 @@ export const _UserAvatar: React.FC<UserAvatarProps> = ({
const statusIndicatorClasses = classNames("UserAvatar__status-indicator", {
"UserAvatar__status-indicator--online": isOnline,
[`UserAvatar__status-indicator--${status}`]: isOnline && status,
"UserAvatar__status-indicator--large": large,
[`UserAvatar__status-indicator--${size}`]: size,
});

const statusIndicatorStyles = useMemo(
Expand Down
2 changes: 1 addition & 1 deletion src/components/molecules/NavBar/NavBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ export const NavBar: React.FC<NavBarPropsType> = ({ hasBackButton = true }) => {
overlay={ProfilePopover}
rootClose={true}
>
<UserAvatar user={userWithId} showStatus medium />
<UserAvatar user={userWithId} showStatus size="medium" />
</OverlayTrigger>
</div>
)}
Expand Down
2 changes: 1 addition & 1 deletion src/components/organisms/ProfileModal/UserInformationContent/UserInformationContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export const UserInformationContent: React.FunctionComponent<UserInformationCont
<h1 className="UserInformationContent__title">My Profile</h1>
<div className="UserInformationContent__information">
<div>
<UserAvatar user={user} showStatus large />
<UserAvatar user={user} showStatus size="large" />
</div>
<div className="UserInformationContent__text-container">
<h3 className="UserInformationContent__user-name">
Expand Down
9 changes: 9 additions & 0 deletions src/scss/constants.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,12 @@ $z-layers: (
overflow: hidden;
text-overflow: ellipsis;
}

@mixin square-size($size: 1, $min: null, $max: null) {
height: $size;
width: $size;
min-height: $min;
min-width: $min;
max-height: $max;
max-width: $max;
}