Skip to content

chore: use emotion for styling (pt. 2) #9951

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 19 commits into from
Oct 2, 2023
+636 −699
Merged

chore: use emotion for styling (pt. 2) #9951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
949c010
parent 4da1223a807e50dd9f307c61bcddd4315dd2ad73
aslilac Sep 29, 2023
3934f0e
emotion: `AvatarData`
aslilac Sep 29, 2023
056d867
emotion: `CopyButton`
aslilac Sep 29, 2023
8808da4
emotion: `DeploymentBannerView`
aslilac Sep 29, 2023
23404c6
use `satisfies`
aslilac Sep 29, 2023
d464bb8
Merge branch 'main' into emotional-damage-2
aslilac Sep 29, 2023
4201a56
emotion: `LicenseBannerView`
aslilac Sep 29, 2023
e3e626a
emotion: `NavbarView`
aslilac Sep 29, 2023
4d68033
emotion: `BorderedMenu`
aslilac Sep 29, 2023
f254256
emotion: `UserDropdownContent`
aslilac Sep 29, 2023
af80a93
emotion: `Badges`
aslilac Sep 29, 2023
ea5bb1b
fix storybook
aslilac Sep 29, 2023
48f3bfc
emotion: `DeploySettingsLayout`
aslilac Sep 29, 2023
2eb0c99
emotion: `Fieldset`
aslilac Sep 29, 2023
8fa8a37
hmm
aslilac Sep 29, 2023
f71a979
fix storybook
aslilac Sep 29, 2023
12fc96c
emotion: `DeploymentSettingsLayout/Fieldset`
aslilac Sep 29, 2023
a446e59
oops
aslilac Sep 29, 2023
e4a6477
tricky
aslilac Sep 29, 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
2 changes: 1 addition & 1 deletion site/src/@types/emotion.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Theme as MuiTheme } from "@mui/system";
import type { DefaultTheme as MuiTheme } from "@mui/system";

declare module "@emotion/react" {
interface Theme extends MuiTheme {}
Expand Down
12 changes: 6 additions & 6 deletions site/src/components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// eslint-disable-next-line no-restricted-imports -- Read above
import MuiAvatar, { AvatarProps as MuiAvatarProps } from "@mui/material/Avatar";
import { FC } from "react";
import { css, type Theme } from "@emotion/react";
import { css, type Interpolation, type Theme } from "@emotion/react";

export type AvatarProps = MuiAvatarProps & {
size?: "sm" | "md" | "xl";
Expand All @@ -11,26 +11,26 @@ export type AvatarProps = MuiAvatarProps & {
};

const sizeStyles = {
sm: (theme: Theme) => ({
sm: (theme) => ({
width: theme.spacing(3),
height: theme.spacing(3),
fontSize: theme.spacing(1.5),
}),
md: {},
xl: (theme: Theme) => ({
xl: (theme) => ({
width: theme.spacing(6),
height: theme.spacing(6),
fontSize: theme.spacing(3),
}),
};
} satisfies Record<string, Interpolation<Theme>>;
Copy link
Member

Choose a reason for hiding this comment

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

Could the Record be updated to be indexed by type Exclude<AvatarProps["size"], undefined>?

Copy link
Member

Choose a reason for hiding this comment

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

Though maybe it'd be better to life the size prop into a separate type, and just reference it in AvatarProps and sizeStyles

Copy link
Member Author

Choose a reason for hiding this comment

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

that's what is cool about satisfies, it doesn't widen the type, it makes sure that T extends Record<...>. if you added an incorrect key here, it would still cause a type error at the lookup, because the keys are still literal types.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's what is cool about satisfies, it doesn't widen the type, it makes sure that T extends Record<...>. if you added an incorrect key here, it would still cause a type error at the lookup, because TS still knows the actual keys the object has.

Copy link
Member

@Parkreiner Parkreiner Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So, you're right that it doesn't widen the type, but I guess the way I look at it is that right now, we have a narrow type, but wide type-checking, when ideally both would be as narrow as possible.

So for this example specifically, there's nothing requiring that all sizes be present in the record – the key for type-checking is any arbitrary string, so TypeScript has no ability to detect typos in key names or forgotten keys. All it knows is that there should be some number of strings in there (including zero)

Like, if we decide to add a "lg" size down the line, there aren't any protections to make sure we don't forget to add a matching style. But with the record being keyed by all members of the union, TypeScript will complain if something doesn't line up

Copy link
Member Author

Choose a reason for hiding this comment

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

there are! the lookup will fail, we don't need a more stringent satisfies to catch that. it'd just make this type harder to write.

Copy link
Member

@Parkreiner Parkreiner Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait, yeah, you're right. I was worried because we don't have the noUncheckedIndexedAccess compiler setting turned on, which can cause issues with trying to read properties from a record, but because it technically isn't a record, and just has the type-checking of one, it's safe.

So we will get errors when trying to consume a property that doesn't exist, instead of weird false positives

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, exactly!


const colorStyles = {
light: {},
darken: (theme: Theme) => ({
darken: (theme) => ({
background: theme.palette.divider,
color: theme.palette.text.primary,
}),
};
} satisfies Record<string, Interpolation<Theme>>;

const fitImageStyles = css`
& .MuiAvatar-img {
Expand Down
65 changes: 34 additions & 31 deletions site/src/components/AvatarData/AvatarData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { FC } from "react";
import { useTheme } from "@emotion/react";
import { Avatar } from "components/Avatar/Avatar";
import { FC } from "react";
import { Stack } from "components/Stack/Stack";
import { makeStyles } from "@mui/styles";

export interface AvatarDataProps {
title: string | JSX.Element;
Expand All @@ -16,7 +16,7 @@ export const AvatarData: FC<AvatarDataProps> = ({
src,
avatar,
}) => {
const styles = useStyles();
const theme = useTheme();

if (!avatar) {
avatar = <Avatar src={src}>{title}</Avatar>;
Expand All @@ -27,38 +27,41 @@ export const AvatarData: FC<AvatarDataProps> = ({
spacing={1.5}
direction="row"
alignItems="center"
className={styles.root}
css={{
minHeight: theme.spacing(5), // Make it predictable for the skeleton
width: "100%",
lineHeight: "150%",
}}
>
{avatar}

<Stack spacing={0} className={styles.info}>
<span className={styles.title}>{title}</span>
{subtitle && <span className={styles.subtitle}>{subtitle}</span>}
<Stack
spacing={0}
css={{
width: "100%",
}}
>
<span
css={{
color: theme.palette.text.primary,
fontWeight: 600,
}}
>
{title}
</span>
{subtitle && (
<span
css={{
fontSize: 12,
color: theme.palette.text.secondary,
lineHeight: "150%",
maxWidth: 540,
}}
>
{subtitle}
</span>
)}
</Stack>
</Stack>
);
};

const useStyles = makeStyles((theme) => ({
root: {
minHeight: theme.spacing(5), // Make it predictable for the skeleton
width: "100%",
lineHeight: "150%",
},

info: {
width: "100%",
},

title: {
color: theme.palette.text.primary,
fontWeight: 600,
},

subtitle: {
fontSize: 12,
color: theme.palette.text.secondary,
lineHeight: "150%",
maxWidth: 540,
},
}));
61 changes: 31 additions & 30 deletions site/src/components/CopyButton/CopyButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import IconButton from "@mui/material/Button";
import { makeStyles } from "@mui/styles";
import Tooltip from "@mui/material/Tooltip";
import Check from "@mui/icons-material/Check";
import { useClipboard } from "hooks/useClipboard";
import { combineClasses } from "utils/combineClasses";
import { css } from "@emotion/react";
import { FileCopyIcon } from "../Icons/FileCopyIcon";

interface CopyButtonProps {
Expand All @@ -29,51 +28,53 @@ export const CopyButton: React.FC<React.PropsWithChildren<CopyButtonProps>> = ({
buttonClassName = "",
tooltipTitle = Language.tooltipTitle,
}) => {
const styles = useStyles();
const { isCopied, copy: copyToClipboard } = useClipboard(text);

const fileCopyIconStyles = css`
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on these style rules being at the top of the component vs at the bottom?

Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer to define things top to bottom, but I'm fine with anything.

Another question, though: is there any difference between having the css tag function defined inside the component vs outside?

Copy link
Member

Choose a reason for hiding this comment

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

I've always put stateful logic (hooks, contexts, queries) at the top of the file while moving CSS to the bottom, but I don't have a strong opinion, just a habit. I'm happy with either. Would be nice to stay consistent either way we go.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I like top-to-bottom. my instinct is always to scroll up for stuff like this, and I'm always a little sad when I realize I was supposed to scroll down. :p

Copy link
Member Author

Choose a reason for hiding this comment

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

although that's probably less of an issue here than it was with makeStyles, because "go to definition" will actually work now 🙃

width: 20px;
height: 20px;
`;

return (
<Tooltip title={tooltipTitle} placement="top">
<div
className={combineClasses([styles.copyButtonWrapper, wrapperClassName])}
className={wrapperClassName}
css={{
display: "flex",
}}
>
<IconButton
className={combineClasses([styles.copyButton, buttonClassName])}
className={buttonClassName}
css={(theme) => css`
border-radius: ${theme.shape.borderRadius}px;
padding: ${theme.spacing(0.85)};
min-width: 32px;

&:hover {
background: ${theme.palette.background.paper};
}
`}
onClick={copyToClipboard}
size="small"
aria-label={Language.ariaLabel}
variant="text"
>
{isCopied ? (
<Check className={styles.fileCopyIcon} />
<Check css={fileCopyIconStyles} />
) : (
<FileCopyIcon className={styles.fileCopyIcon} />
<FileCopyIcon css={fileCopyIconStyles} />
)}
{ctaCopy && (
<div
css={(theme) => ({
marginLeft: theme.spacing(1),
})}
>
{ctaCopy}
</div>
)}
{ctaCopy && <div className={styles.buttonCopy}>{ctaCopy}</div>}
</IconButton>
</div>
</Tooltip>
);
};

const useStyles = makeStyles((theme) => ({
copyButtonWrapper: {
display: "flex",
},
copyButton: {
borderRadius: theme.shape.borderRadius,
padding: theme.spacing(0.85),
minWidth: 32,

"&:hover": {
background: theme.palette.background.paper,
},
},
fileCopyIcon: {
width: 20,
height: 20,
},
buttonCopy: {
marginLeft: theme.spacing(1),
},
}));
Loading