Skip to content

refactor(site): Normalize avatar components #5860

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 26, 2023
Merged

refactor(site): Normalize avatar components #5860

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
27ca826
refactor(site): Normalize avatar components
BrunoQuaresma Jan 25, 2023
72bfda0
Fix template layout size
BrunoQuaresma Jan 25, 2023
1cb58da
No need for sm styles
BrunoQuaresma Jan 25, 2023
900622e
Add storybook for Avatar
BrunoQuaresma Jan 25, 2023
454a1f5
Fix fmt
BrunoQuaresma Jan 25, 2023
61b5f95
Improvements nd fixes
BrunoQuaresma Jan 26, 2023
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
Next Next commit
refactor(site): Normalize avatar components
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Jan 25, 2023
commit 27ca82648c933a7d57b51b3f2dbcb0e00855dba6
3 changes: 3 additions & 0 deletions site/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ rules:
message:
"Use path imports to avoid pulling in unused modules. See:
https://material-ui.com/guides/minimizing-bundle-size/"
- name: "@material-ui/core/Avatar"
message:
"You should use the Avatar component provided on components/Avatar/Avatar"
no-unused-vars: "off"
"object-curly-spacing": "off"
react-hooks/exhaustive-deps: warn
Expand Down
79 changes: 79 additions & 0 deletions site/src/components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// This is the only place MuiAvatar can be used
// eslint-disable-next-line no-restricted-imports -- Read above
import MuiAvatar, {
AvatarProps as MuiAvatarProps,
} from "@material-ui/core/Avatar"
import { makeStyles } from "@material-ui/core/styles"
import { cloneElement, FC } from "react"
import { combineClasses } from "util/combineClasses"
import { firstLetter } from "./firstLetter"

export type AvatarProps = MuiAvatarProps & {
size?: "sm" | "md" | "xl"
colorScheme?: "light" | "darken"
fitImage?: boolean
}

export const Avatar: FC<AvatarProps> = ({
size = "md",
colorScheme = "light",
fitImage,
className,
children,
...muiProps
}) => {
const styles = useStyles()

return (
<MuiAvatar
{...muiProps}
className={combineClasses([
className,
styles[size],
styles[colorScheme],
fitImage && styles.fitImage,
])}
>
{/* If the children is a string, we always want to render the first letter */}
{typeof children === "string" ? firstLetter(children) : children}
</MuiAvatar>
)
}

export const AvatarIcon: FC<{ children: JSX.Element }> = ({ children }) => {
const styles = useStyles()
return cloneElement(children, { className: styles.avatarIcon })
}

const useStyles = makeStyles((theme) => ({
// Size styles
sm: {
width: theme.spacing(4),
height: theme.spacing(4),
fontSize: theme.spacing(2),
},
// Just use the default value from theme
md: {},
xl: {
width: theme.spacing(6),
height: theme.spacing(6),
fontSize: theme.spacing(3),
},
// Colors
// Just use the default value from theme
light: {},
darken: {
background: theme.palette.divider,
color: theme.palette.text.primary,
},
// Avatar icon
avatarIcon: {
maxWidth: "50%",
},
// Fit image
fitImage: {
"& .MuiAvatar-img": {
objectFit: "contain",
},
},
}))
0 site/src/util/firstLetter.test.ts → ...src/components/Avatar/firstLetter.test.ts
View file
Open in desktop
File renamed without changes.
0 site/src/util/firstLetter.ts → site/src/components/Avatar/firstLetter.ts
View file
Open in desktop
File renamed without changes.
67 changes: 23 additions & 44 deletions site/src/components/AvatarData/AvatarData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,71 +1,50 @@
import Avatar from "@material-ui/core/Avatar"
import Link from "@material-ui/core/Link"
import { makeStyles } from "@material-ui/core/styles"
import { Avatar } from "components/Avatar/Avatar"
import { FC, PropsWithChildren } from "react"
import { Link as RouterLink } from "react-router-dom"
import { firstLetter } from "../../util/firstLetter"
import {
TableCellData,
TableCellDataPrimary,
TableCellDataSecondary,
} from "../TableCellData/TableCellData"
import { Stack } from "components/Stack/Stack"
import { makeStyles } from "@material-ui/core/styles"

export interface AvatarDataProps {
title: string
subtitle?: string
highlightTitle?: boolean
link?: string
src?: string
avatar?: React.ReactNode
Copy link
Member

Choose a reason for hiding this comment

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

This is not really important, just a thought but would it make sense to make AvatarData take Avatar props and pass them along rather than accept avatar? My reasoning is that it seems unfortunate that there are two ways of doing things like:

<AvatarData src={src} />

and

<AvatarData avatar={<Avatar src={src} />} />

Or alternatively we could always make it so we always have to pass an avatar, either way works so there is one way of doing things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AvatarData is a bad name for what it does and not a good abstraction. Wondering if Heading or DataHeading would be better with the following interface:

<Heading
  title="Bruno"
  subtitle="bruno@hotmail.com"
  leftElement={<UserAvatar user="{user}" />}
/>

Copy link
Member

Choose a reason for hiding this comment

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

Definitely agree!

}

export const AvatarData: FC<PropsWithChildren<AvatarDataProps>> = ({
title,
subtitle,
link,
highlightTitle,
src,
avatar,
}) => {
const styles = useStyles()

if (!avatar) {
avatar = <Avatar>{firstLetter(title)}</Avatar>
avatar = <Avatar src={src}>{title}</Avatar>
}

return (
<div className={styles.root}>
<div className={styles.avatarWrapper}>{avatar}</div>
<Stack spacing={1.5} direction="row" alignItems="center">
{avatar}

{link ? (
<Link to={link} underline="none" component={RouterLink}>
<TableCellData>
<TableCellDataPrimary highlight={highlightTitle}>
{title}
</TableCellDataPrimary>
{subtitle && (
<TableCellDataSecondary>{subtitle}</TableCellDataSecondary>
)}
</TableCellData>
</Link>
) : (
<TableCellData>
<TableCellDataPrimary highlight={highlightTitle}>
{title}
</TableCellDataPrimary>
{subtitle && (
<TableCellDataSecondary>{subtitle}</TableCellDataSecondary>
)}
</TableCellData>
)}
</div>
<Stack spacing={0}>
<span className={styles.title}>{title}</span>
{subtitle && <span className={styles.subtitle}>{subtitle}</span>}
</Stack>
</Stack>
)
}

const useStyles = makeStyles((theme) => ({
root: {
display: "flex",
alignItems: "center",
title: {
color: theme.palette.text.primary,
fontWeight: 600,
},
avatarWrapper: {
marginRight: theme.spacing(1.5),

subtitle: {
fontSize: 12,
color: theme.palette.text.secondary,
lineHeight: "140%",
marginTop: 2,
maxWidth: 540,
},
}))
28 changes: 5 additions & 23 deletions site/src/components/BuildsTable/BuildAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Avatar from "@material-ui/core/Avatar"
import Badge from "@material-ui/core/Badge"
import { Theme, useTheme, withStyles } from "@material-ui/core/styles"
import { FC } from "react"
Expand All @@ -8,6 +7,7 @@ import DeleteOutlined from "@material-ui/icons/DeleteOutlined"
import { WorkspaceBuild, WorkspaceTransition } from "api/typesGenerated"
import { getDisplayWorkspaceBuildStatus } from "util/workspace"
import { PaletteIndex } from "theme/palettes"
import { Avatar, AvatarProps } from "components/Avatar/Avatar"

interface StylesBadgeProps {
type: PaletteIndex
Expand All @@ -25,27 +25,9 @@ const StyledBadge = withStyles((theme) => ({
},
}))(Badge)

interface StyledAvatarProps {
size?: number
}

const StyledAvatar = withStyles((theme) => ({
root: {
background: theme.palette.divider,
color: theme.palette.text.primary,
border: `2px solid ${theme.palette.divider}`,
width: ({ size }: StyledAvatarProps) => size,
height: ({ size }: StyledAvatarProps) => size,

"& svg": {
width: ({ size }: StyledAvatarProps) => (size ? size / 2 : 18),
height: ({ size }: StyledAvatarProps) => (size ? size / 2 : 18),
},
},
}))(Avatar)

export interface BuildAvatarProps extends StyledAvatarProps {
export interface BuildAvatarProps {
build: WorkspaceBuild
size?: AvatarProps["size"]
}

const iconByTransition: Record<WorkspaceTransition, JSX.Element> = {
Expand All @@ -71,9 +53,9 @@ export const BuildAvatar: FC<BuildAvatarProps> = ({ build, size }) => {
}}
badgeContent={<div></div>}
>
<StyledAvatar size={size}>
<Avatar size={size} colorScheme="darken">
{iconByTransition[build.transition]}
</StyledAvatar>
</Avatar>
</StyledBadge>
)
}
5 changes: 2 additions & 3 deletions site/src/components/GroupAvatar/GroupAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import Avatar from "@material-ui/core/Avatar"
import { Avatar } from "components/Avatar/Avatar"
import Badge from "@material-ui/core/Badge"
import { withStyles } from "@material-ui/core/styles"
import Group from "@material-ui/icons/Group"
import { FC } from "react"
import { firstLetter } from "util/firstLetter"

const StyledBadge = withStyles((theme) => ({
badge: {
Expand Down Expand Up @@ -38,7 +37,7 @@ export const GroupAvatar: FC<GroupAvatarProps> = ({ name, avatarURL }) => {
}}
badgeContent={<Group />}
>
<Avatar src={avatarURL}>{firstLetter(name)}</Avatar>
<Avatar src={avatarURL}>{name}</Avatar>
</StyledBadge>
)
}
24 changes: 8 additions & 16 deletions site/src/components/Resources/ResourceAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import Avatar from "@material-ui/core/Avatar"
import { makeStyles } from "@material-ui/core/styles"
import { Avatar, AvatarIcon } from "components/Avatar/Avatar"
import { FC } from "react"
import { WorkspaceResource } from "../../api/typesGenerated"

const FALLBACK_ICON = "/icon/widgets.svg"

// NOTE @jsjoeio, @BrunoQuaresma
// These resources (i.e. docker_image, kubernetes_deployment) map to Terraform
// resource types. These are the most used ones and are based on user usage.
// We may want to update from time-to-time.
Expand Down Expand Up @@ -37,18 +35,12 @@ export type ResourceAvatarProps = { resource: WorkspaceResource }
export const ResourceAvatar: FC<ResourceAvatarProps> = ({ resource }) => {
const hasIcon = resource.icon && resource.icon !== ""
const avatarSrc = hasIcon ? resource.icon : getIconPathResource(resource.type)
const styles = useStyles()

return <Avatar className={styles.resourceAvatar} src={avatarSrc} />
return (
<Avatar colorScheme="darken">
<AvatarIcon>
<img src={avatarSrc} alt="" />
Copy link
Member

Choose a reason for hiding this comment

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

Just to check I follow: the difference between <Avatar src={src} /> and <Avatar><img src={src} /></Avatar> is that the former shows the icon only and the former shows the icon wrapped in a circle, is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The difference is: when you pass the image as a child, it does not add the MuiAvatar-img class to it so it is just an image inside of a rounded div without any style. My idea for the AvatarIcon is to make the image behaves like a MuiIcon.

Thinking better, I have no idea why I decided to use a child instead of using the AvatarIcon directly like:

// Instead of
<AvatarIcon>
   <img src={avatarSrc} alt="" />
</AvatarIcon>

// it should be
<AvatarIcon src={avatarSrc} />

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh ok yeah that makes sense!

</AvatarIcon>
</Avatar>
)
}

const useStyles = makeStyles((theme) => ({
resourceAvatar: {
backgroundColor: theme.palette.divider,

"& img": {
width: 18,
height: 18,
},
},
}))
43 changes: 0 additions & 43 deletions site/src/components/TableCellData/TableCellData.tsx
View file
Open in desktop

This file was deleted.

25 changes: 7 additions & 18 deletions site/src/components/TemplateLayout/TemplateLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Avatar from "@material-ui/core/Avatar"
import Button from "@material-ui/core/Button"
import Link from "@material-ui/core/Link"
import { makeStyles } from "@material-ui/core/styles"
Expand All @@ -19,7 +18,6 @@ import {
useParams,
} from "react-router-dom"
import { combineClasses } from "util/combineClasses"
import { firstLetter } from "util/firstLetter"
import {
TemplateContext,
templateMachine,
Expand All @@ -29,6 +27,7 @@ import { Stack } from "components/Stack/Stack"
import { Permissions } from "xServices/auth/authXService"
import { Loader } from "components/Loader/Loader"
import { usePermissions } from "hooks/usePermissions"
import { Avatar } from "components/Avatar/Avatar"

const Language = {
settingsButton: "Settings",
Expand Down Expand Up @@ -139,17 +138,12 @@ export const TemplateLayout: FC<{ children?: JSX.Element }> = ({
}
>
<Stack direction="row" spacing={3} className={styles.pageTitle}>
<div>
{hasIcon ? (
<div className={styles.iconWrapper}>
<img src={template.icon} alt="" />
</div>
) : (
<Avatar className={styles.avatar}>
{firstLetter(template.name)}
</Avatar>
)}
</div>
{hasIcon ? (
<Avatar src={template.icon} variant="square" fitImage />
) : (
<Avatar size="xl">{template.name}</Avatar>
)}

<div>
<PageHeaderTitle>
{template.display_name.length > 0
Expand Down Expand Up @@ -212,11 +206,6 @@ export const useStyles = makeStyles((theme) => {
pageTitle: {
alignItems: "center",
},
avatar: {
width: theme.spacing(6),
height: theme.spacing(6),
fontSize: theme.spacing(3),
},
iconWrapper: {
width: theme.spacing(6),
height: theme.spacing(6),
Expand Down
Loading