Skip to content

fix: update component styling to defer more to MUI theme #143

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+69 −23
Open

fix: update component styling to defer more to MUI theme #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edbcd9d
fix: update all font sizes to be defined by theme
Parkreiner Oct 17, 2024
fb4990b
fix: finish updating styles for components
Parkreiner Oct 17, 2024
3386d42
fix: revert temp config change
Parkreiner Oct 17, 2024
40aaa11
fix: update styles to be more based on body values
Parkreiner Oct 18, 2024
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
6 changes: 2 additions & 4 deletions plugins/backstage-plugin-coder/src/components/A11yInfoCard/A11yInfoCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
import React, { type HTMLAttributes, type ReactNode, forwardRef } from 'react';
import { makeStyles } from '@material-ui/core';
import { scaleCssUnit } from '../../utils/styling';

export type A11yInfoCardProps = Readonly<
HTMLAttributes<HTMLDivElement> & {
Expand All @@ -23,10 +24,7 @@ const useStyles = makeStyles(theme => ({
},

headerContent: {
// Ideally wouldn't be using hard-coded font sizes, but couldn't figure out
// how to use the theme.typography property, especially since not all
// sub-properties have font sizes defined
fontSize: '1.5rem',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 1.5),
color: theme.palette.text.primary,
fontWeight: 700,
borderBottom: `1px solid ${theme.palette.divider}`,
Expand Down
3 changes: 3 additions & 0 deletions plugins/backstage-plugin-coder/src/components/CoderAuthForm/CoderAuthForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ export const CoderAuthForm = ({ descriptionId }: CoderAuthFormProps) => {
return <CoderAuthInputForm />;
}

// It shouldn't be possible for these branches to run, because parent
// code should show main content instead of the form when the user is
// authenticated. Still adding them for extra assurance
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍

case 'authenticated':
case 'distrustedWithGracePeriod': {
return <CoderAuthSuccessStatus />;
Expand Down
12 changes: 4 additions & 8 deletions plugins/backstage-plugin-coder/src/components/CoderAuthForm/CoderAuthSuccessStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const useStyles = makeStyles(theme => ({
marginLeft: 'auto',
marginRight: 'auto',
color: theme.palette.text.primary,
fontSize: '1rem',
fontSize: theme.typography.body1.fontSize,
},

statusArea: {
Expand All @@ -35,13 +35,9 @@ const useStyles = makeStyles(theme => ({
alignItems: 'center',
},

logo: {
//
},

text: {
textAlign: 'center',
lineHeight: '1rem',
lineHeight: theme.typography.body1.fontSize,
},
}));

Expand All @@ -51,8 +47,8 @@ export function CoderAuthSuccessStatus() {
return (
<div className={styles.root}>
<div className={styles.statusArea}>
<CoderLogo className={styles.logo} />
<p className={styles.text}>You are fully authenticated with Coder!</p>
<CoderLogo />
<p className={styles.text}>You are fully authenticated with Coder.</p>
</div>

<UnlinkAccountButton />
Expand Down
7 changes: 6 additions & 1 deletion plugins/backstage-plugin-coder/src/components/CoderAuthFormDialog/CoderAuthFormDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import DialogContent from '@material-ui/core/DialogContent';
import DialogTitle from '@material-ui/core/DialogTitle';
import DialogActions from '@material-ui/core/DialogActions';
import { CoderAuthForm } from '../CoderAuthForm/CoderAuthForm';
import { scaleCssUnit } from '../../utils/styling';

const useStyles = makeStyles(theme => ({
trigger: {
Expand Down Expand Up @@ -41,7 +42,7 @@ const useStyles = makeStyles(theme => ({
},

dialogTitle: {
fontSize: '24px',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 1.5),
borderBottom: `${theme.palette.divider} 1px solid`,
padding: `${theme.spacing(1)}px ${theme.spacing(3)}px`,
},
Expand All @@ -60,6 +61,10 @@ const useStyles = makeStyles(theme => ({
},

closeButton: {
// MUI's typography object doesn't expose any letter tracking values, even
// though you need them to make sure that all-caps text doesn't bunch up.
// Even if the text of the button changes, the styles might look slightly
// wonky, but they won't cause any obvious readability/styling issues
letterSpacing: '0.05em',
padding: `${theme.spacing(0.5)}px ${theme.spacing(1)}px`,
color: theme.palette.primary.main,
Expand Down
2 changes: 1 addition & 1 deletion plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/CreateWorkspaceLink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const useStyles = makeStyles<Theme, StyleInput, StyleKeys>(theme => {
justifyContent: 'center',
alignItems: 'center',
backgroundColor: 'inherit',
borderRadius: '9999px',
borderRadius: theme.shape.borderRadius,
lineHeight: 1,
color: canCreateWorkspace
? theme.palette.text.primary
Expand Down
2 changes: 1 addition & 1 deletion plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/ExtraActionsButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const useStyles = makeStyles(theme => {
width: theme.spacing(4) + padding,
height: theme.spacing(4) + padding,
border: 'none',
borderRadius: '9999px',
borderRadius: theme.shape.borderRadius,
backgroundColor: 'inherit',
lineHeight: 1,

Expand Down
7 changes: 5 additions & 2 deletions plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/HeaderRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { HTMLAttributes, ReactNode } from 'react';
import { type Theme, makeStyles } from '@material-ui/core';
import { useWorkspacesCardContext } from './Root';
import type { HtmlHeader } from '../../typesConstants';
import { scaleCssUnit } from '../../utils/styling';

type StyleKey = 'root' | 'header' | 'hgroup' | 'subheader';
const useStyles = makeStyles<Theme, {}, StyleKey>(theme => ({
Expand All @@ -18,14 +19,16 @@ const useStyles = makeStyles<Theme, {}, StyleKey>(theme => ({
},

header: {
fontSize: '1.5rem',
// Setting line height solid (when leading and font size are equal) to make
// it easier for header and subtitle to sit next to each other
lineHeight: 1,
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 1.5),
margin: 0,
},

subheader: {
margin: '0',
fontSize: '0.875rem',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 0.875),
fontWeight: 400,
color: theme.palette.text.secondary,
paddingTop: theme.spacing(0.5),
Expand Down
3 changes: 2 additions & 1 deletion plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/Placeholder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { useWorkspacesCardContext } from './Root';
import { makeStyles } from '@material-ui/core';
import { CoderLogo } from '../CoderLogo';
import { VisuallyHidden } from '../VisuallyHidden';
import { scaleCssUnit } from '../../utils/styling';

const usePlaceholderStyles = makeStyles(theme => ({
root: {
Expand All @@ -24,7 +25,7 @@ const usePlaceholderStyles = makeStyles(theme => ({
textAlign: 'center',
padding: `0 ${theme.spacing(2.5)}px`,
fontWeight: 400,
fontSize: '1.125rem',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 1.125),
color: theme.palette.text.secondary,
lineHeight: 1.1,
},
Expand Down
5 changes: 3 additions & 2 deletions plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/SearchBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Theme, makeStyles } from '@material-ui/core';
import { useWorkspacesCardContext } from './Root';
import SearchIcon from '@material-ui/icons/Search';
import CloseIcon from '@material-ui/icons/Close';
import { CUSTOM_FOCUS_COLOR } from '../../utils/styling';

const LABEL_TEXT = 'Search your Coder workspaces';
const SEARCH_DEBOUNCE_MS = 400;
Expand Down Expand Up @@ -50,7 +51,7 @@ const useStyles = makeStyles<Theme, MakeStylesInput, StyleKey>(theme => ({
},

'&:focus-within': {
boxShadow: '0 0 0 1px hsl(213deg, 94%, 68%)',
boxShadow: `0 0 0 1px ${CUSTOM_FOCUS_COLOR}`,
},

// Makes it so that the container doesn't have visible focus while you're
Expand Down Expand Up @@ -93,7 +94,7 @@ const useStyles = makeStyles<Theme, MakeStylesInput, StyleKey>(theme => ({
cursor: 'pointer',

'&:focus': {
boxShadow: '0 0 0 1px hsl(213deg, 94%, 68%)',
boxShadow: `0 0 0 1px ${CUSTOM_FOCUS_COLOR}`,
},
}),
}));
Expand Down
3 changes: 2 additions & 1 deletion plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/WorkspacesListIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { ForwardedRef, HTMLAttributes, useState } from 'react';
import { useUrlSync } from '../../hooks/useUrlSync';
import { Theme, makeStyles } from '@material-ui/core';
import { scaleCssUnit } from '../../utils/styling';

type WorkspaceListIconProps = Readonly<
Omit<HTMLAttributes<HTMLDivElement>, 'children' | 'aria-hidden'> & {
Expand All @@ -21,7 +22,7 @@ const useStyles = makeStyles<Theme, MakeStylesInput, StyleKey>(theme => ({
root: {
width: theme.spacing(2.5),
height: theme.spacing(2.5),
fontSize: '0.625rem',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 0.625),
backgroundColor: theme.palette.background.default,
borderRadius: '9999px',
display: 'flex',
Expand Down
2 changes: 1 addition & 1 deletion plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/WorkspacesListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const useStyles = makeStyles<Theme, UseStyleInputs, StyleKey>(theme => ({
alignItems: 'center',
gap: theme.spacing(1),
color: theme.palette.text.secondary,
fontSize: '16px',
fontSize: theme.typography.body1.fontSize ?? '1rem',
},

onlineStatusLight: ({ isAvailable }) => ({
Expand Down
3 changes: 2 additions & 1 deletion plugins/backstage-plugin-coder/src/components/Disclosure/Disclosure.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import React, { type HTMLAttributes, type ReactNode, useState } from 'react';
import { useId } from '../../hooks/hookPolyfills';
import { makeStyles } from '@material-ui/core';
import { scaleCssUnit } from '../../utils/styling';

const useStyles = makeStyles(theme => ({
disclosureTriangle: {
display: 'inline-block',
textAlign: 'right',
width: theme.spacing(2.25),
fontSize: '0.7rem',
fontSize: scaleCssUnit(theme.typography.body1.fontSize, 0.7),
},

disclosureBody: {
Expand Down
37 changes: 37 additions & 0 deletions plugins/backstage-plugin-coder/src/utils/styling.ts
View file
Open in desktop
Copy link
Member Author

@Parkreiner Parkreiner Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wish I didn't need to make a function like this, but it really felt like it was the best way to ensure that the theme logic doesn't randomly break.

One of the problems with the base MUI theme object that Backstage gives you by default is that it's light on the number of font sizes. You generally want very few sizes for longform text, but for UI, you need more variety to have better information hierarchy

I was able to get pretty close to the old design without needing this function, but then I needed to access weird properties like theme.typography.h5.fontSize, and:

  1. Saying that you want your h2 to be the size of an h5 feels weird and fragile
  2. Realistically, I don't expect many MUI users to actually get so detailed with their theme configuration that they'll ever touch the h5 property, so the code is more likely to be out of sync, even though that defeats the point of a theme

I think it's safe to say that most users will at least look at body1 and body2, which is why I made it a bit easier to base custom sizes off of them

Copy link
Member

Choose a reason for hiding this comment

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

I might be on the wrong page, but should we not just use h2.fontSize for h2, even if it looks wonky? Or alternatively, does this mean we should actually be using an h5 instead of an h2? My impression is that we should not try to work around MUI's problems, that is something for Backstage to sort out. 😆

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* A custom focus color chosen to look close to a system default, while
* remaining visible in dark and light themes. The focus values from Backstage's
* theme object are too low-contrast to meet accessibility requirements.
*/
export const CUSTOM_FOCUS_COLOR = 'hsl(213deg, 94%, 68%)';

export function scaleCssUnit(
baseSize: string | number | undefined,
scale: number,
): string {
if (!Number.isFinite(scale)) {
return '1rem';
}

if (baseSize === undefined) {
return `${scale}rem`;
}

if (typeof baseSize === 'number') {
if (!Number.isFinite(baseSize)) {
return `${scale}rem`;
}

return `${baseSize * scale}px`;
}

const sizeRe = /^\s*(?<value>\d+(?:\.\d+))?s*(?<unit>px|r?em)\s*$/i;
Copy link

Choose a reason for hiding this comment

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

I'm taking your regular expressions away it's for your own good.

const { value, unit } = sizeRe.exec(baseSize)?.groups ?? {};
const numValue = Number(value);

if (Number.isNaN(numValue) || unit === undefined) {
return `${scale}rem`;
}

return `${scale * numValue}${unit}`;
}