fix(site): standardize headers for Admin Settings page #16911

Parkreiner · 2025-03-13T16:36:26Z

No issue to link – it's just a small thing that has been on my radar for a bit

Changes made

Switched almost all headers to use the SettingHeader component
Redesigned component to be more composition-based, to stay in line with the patterns we're starting to use more throughout the codebase
Refactored SettingHeader to be based on Radix and Tailwind, rather than Emotion/MUI
Added additional props to SettingHeader to help resolve issues with the component creating invalid HTML
Beefed up SettingHeader to have better out-of-the-box accessibility
Addressed some typographic problems in SettingHeader
Addressed some responsive layout problems for SettingsHeader
Added first-ever stories for SettingsHeader

Notes

There are still a few headers that aren't using SettingHeader yet. There were some UI edge cases that meant I couldn't reliably bring it in without consulting the Design team first. I'm a little less worried about them, because they at least look like the other headers, but it'd be nice if we could centralize everything in a followup PR

Screenshots

Workspace Proxies

Before – Main header does not match the style of the other pages

After

External Authentication

Before – The max width for the description text is too narrow. Body text for English-language text has an ideal width of 50em–75em, which is an agreed-upon standard that has existed since the 1600s. Most graphic designers and typographers tend to default to 65em

After

Parkreiner · 2025-03-13T20:40:03Z

site/src/components/SettingsHeader/SettingsHeader.tsx

-		<Stack alignItems="baseline" direction="row" justifyContent="space-between">
-			<div css={{ maxWidth: 420, marginBottom: 24 }}>
-				<Stack direction="row" spacing={1} alignItems="center">
-					<h1


This was a big accessibility/HTML validity issue. We were using this component multiple times on the same page, but only one h1 is allowed to be on a page, total

This is why I decoupled the secondary boolean into a styling prop and a semantic prop. There may be cases when we need to change the heading level to make the HTML valid, even though the styling should be exactly the same

Parkreiner · 2025-03-13T20:45:40Z

site/src/components/SettingsHeader/SettingsHeader.tsx

+				<div className="flex flex-row gap-2 align-middle">
+					<HeaderTitle
+						className={twMerge(
+							"m-0 pb-1 text-3xl font-bold flex items-center gap-2 leading-tight",


The different leading values for the header and description are intentional – leading needs to go down the larger your text is

Parkreiner · 2025-03-13T20:46:18Z

site/src/pages/DeploymentSettingsPage/NotificationsPage/NotificationsPage.tsx

@@ -47,7 +47,7 @@ export const NotificationsPage: FC = () => {
 				title={
 					<>
 						Notifications
-						<span css={{ position: "relative", top: "-6px" }}>
+						<span css={{ position: "relative", top: "2px" }}>


Needed to adjust this slightly to account for the updated text leading

Is it something we could use CSS variables and calc to make the top value more reliable and easier to understand? By understand I mean, why is it 2px? If not, a comment would be nice.

Parkreiner · 2025-03-13T20:47:05Z

site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx

-				"& code": {
-					background: theme.palette.divider,
-					fontSize: 12,
-					padding: "2px 4px",
-					color: theme.palette.text.primary,
-					borderRadius: 2,
-				},


This code seemed to be part of a previous version of the component. None of the styling ever applied to anything

Parkreiner · 2025-03-13T20:48:03Z

site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx

@@ -17,29 +13,14 @@ export const WorkspaceProxyPage: FC = () => {
 	} = useProxy();

 	return (
-		<Section


This was deleted because the header is now located in the view, which I think is how it should've always been

Parkreiner · 2025-03-13T20:49:28Z

@jaaydenh I'm tagging you for review, but it's not super important that you get to this anytime soon. This is the PR I'm going to be using for the Agentic experiments, and I'm also off tomorrow

Parkreiner · 2025-03-14T22:55:26Z

site/src/components/SettingsHeader/SettingsHeader.tsx


-interface HeaderProps {
+type HeaderHierarchy = "primary" | "secondary";


Made it like this so that we can add additional variants over time

For creating variants, I would recommend you to use class-variance-authority package. You can see how it is used in the Button component.

Yeah, I've used CVA before – I actually used it in the take-home that got me into Coder. I thought it was a bit too much overhead here, but if we're switching to CVA as a go-to, I can swap that in

Because of shadcn, our base components are using CVA so I think it would be nice to use it but it is up to you.

BrunoQuaresma · 2025-03-19T12:51:08Z

site/src/components/SettingsHeader/SettingsHeader.tsx


-interface HeaderProps {
+type HeaderHierarchy = "primary" | "secondary";


For creating variants, I would recommend you to use class-variance-authority package. You can see how it is used in the Button component.

BrunoQuaresma · 2025-03-19T12:52:27Z

site/src/components/SettingsHeader/SettingsHeader.tsx

 }) => {
-	const theme = useTheme();
-
+	const HeaderTitle = titleHeaderLevel;


Instead of doing this, we have access to the Slot component from Radix. I think it is a better tool for this kinda of "polymorphic" component.

I don't know if that makes sense here. Slot allows any component to be slotted in as a child – it's too permissive with what it lets you compose into it, when there should only ever be six possible values (h1–h6), and all six have the exact same underlying node type

The current setup is a more restricted form of polymorphism

BrunoQuaresma · 2025-03-19T12:54:05Z

site/src/components/SettingsHeader/SettingsHeader.tsx

 	title: ReactNode;
 	description?: ReactNode;
-	secondary?: boolean;
+	titleVisualHierarchy?: HeaderHierarchy;
+	titleHeaderLevel?: HeaderLevel;


I know it is how we did before but since we are touching this, could we also improve the abstraction? I see something like:

<SettingsHeader actions={[<SeeDocsButton />]}> <SettingsHeaderTitle>Mytitle</SettingsHeaderTitle> <SettingsHeaderSubtitle>Some text hear</SettingsHeaderSubtitle> </SettingsHeader>

BrunoQuaresma · 2025-03-19T12:55:04Z

site/src/components/SettingsHeader/SettingsHeader.tsx

Can we add a storybook for it covering the new variants?

Parkreiner · 2025-03-28T17:42:22Z

@BrunoQuaresma Re-requesting your review – I just have some small micro-adjustments to make for three stories. I went ahead and applied your feedback as much as I could – I still feel like bringing in Slot is the wrong move and increases the risks of the component being misused

You'd be a hero if you could get this done today, but if not, I'll go ahead and make a hotfix over the weekend for adjusting the wonky headers that spurred this PR. I just want to make sure we don't ship wonky headers three releases in a row

Parkreiner · 2025-03-28T18:03:29Z

site/src/components/SettingsHeader/SettingsHeader.tsx

+	// Explicitly not using Radix's Slot component, because we don't want to
+	// allow any arbitrary element to be composed into this. We specifically
+	// only want to allow the six HTML headers. Anything else will likely result
+	// in invalid markup


Added a comment to call out why the code isn't using Slot

BrunoQuaresma

If the snapshots look good, go for it!

Parkreiner added 6 commits March 11, 2025 14:06

fix: migrate deployment workspace proxies page to new header style

de69ee3

fix: update other proxy page to use proper render key values

a1da1c1

fix: make secondary styling more obvious

e5d581b

refactor: move SettingsHeader to Tailwind

9b4928b

fix: resolve header level bugs

a35a9ae

fix: constrain max width

9872c5d

Parkreiner self-assigned this Mar 13, 2025

Parkreiner changed the title ~~[DO NOT MERGE] fix(site): standardize how headers are displayed for Admin Settings~~ fix(site): standardize how headers are displayed for Admin Settings [DO NOT MERGE] Mar 13, 2025

Parkreiner changed the title ~~fix(site): standardize how headers are displayed for Admin Settings [DO NOT MERGE]~~ fix(site): standardize how headers are displayed for Admin Settings page [DO NOT MERGE] Mar 13, 2025

Parkreiner added 5 commits March 13, 2025 17:37

fix: apply formatting

1fe0022

fix: update HTML semantics

7d0903a

fix: flush semantics fix

a512bf5

fix: update leading

734f59d

fix: resolve styling edge case for beta badge

49b9dfe

Parkreiner commented Mar 13, 2025

View reviewed changes

Parkreiner requested a review from jaaydenh March 13, 2025 20:48

fix: set correct font size

840d6df

Parkreiner commented Mar 14, 2025

View reviewed changes

fix: remove redundant class

f169c4e

Parkreiner changed the title ~~fix(site): standardize how headers are displayed for Admin Settings page [DO NOT MERGE]~~ fix(site): standardize headers for Admin Settings page [DO NOT MERGE] Mar 17, 2025

BrunoQuaresma reviewed Mar 19, 2025

View reviewed changes

github-actions bot added the stale This issue is like stale bread. label Mar 27, 2025

Merge branch 'main' into mes/headers-fix-b

2325982

Parkreiner changed the title ~~fix(site): standardize headers for Admin Settings page [DO NOT MERGE]~~ fix(site): standardize headers for Admin Settings page Mar 28, 2025

refactor: switch to CVA

99c2dbc

Parkreiner added 4 commits March 28, 2025 16:48

refactor: update API design of SettingsHeader

50eddd0

chore: add first-ever stories for SettingsHeader

2b77784

fix: make header more responsive

e2523cb

fix: format

7f53666

Parkreiner removed the stale This issue is like stale bread. label Mar 28, 2025

Parkreiner requested a review from BrunoQuaresma March 28, 2025 17:40

fix: update beta badge styling for Notifications page

daf6da9

Parkreiner commented Mar 28, 2025

View reviewed changes

BrunoQuaresma approved these changes Apr 1, 2025

View reviewed changes

Parkreiner merged commit fcac4ab into main Apr 1, 2025
30 checks passed

Parkreiner deleted the mes/headers-fix-b branch April 1, 2025 14:13

github-actions bot locked and limited conversation to collaborators Apr 1, 2025


		interface HeaderProps {
		type HeaderHierarchy = "primary" \| "secondary";

fix(site): standardize headers for Admin Settings page #16911

fix(site): standardize headers for Admin Settings page #16911

Uh oh!

Conversation

Parkreiner commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Notes

Screenshots

Workspace Proxies

External Authentication

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Parkreiner commented Mar 13, 2025 •

edited

Loading

BrunoQuaresma Apr 1, 2025 •

edited

Loading

Parkreiner Mar 13, 2025 •

edited

Loading

Parkreiner Mar 28, 2025 •

edited

Loading

Parkreiner commented Mar 28, 2025 •

edited

Loading