fix: resolve heading sizing by jakehwll · Pull Request #21914

fix: resolve heading sizing by jakehwll · Pull Request #21914 · coder/coder

This pull-request addresses heading sizing inline with Figma. This means that our headings are all uniformly font-weight and font-size. Furthermore, we've dropped the font-size of the descriptions below the headings.

Comparison

Old	New

This one is harder to see, but its mild spacing resolution 🙂

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9acfc63330

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Choose a reason for hiding this comment

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

Code Review

Reviewed heading sizing unification changes. Found 4 issues that need attention:

Breaking change: condensed prop removed from implementation but still used in 3 files
Duplicate class: gap-8 appears twice in PageHeader
Invalid flex value: flex-start should be items-start
Font weight changes: Verify PageHeaderCaption and SettingsHeaderDescription changes match Figma

The Emotion → Tailwind migration looks good overall, but these issues need fixing before merge.

Comment on lines 50 to 61


		export const PageHeaderSubtitle: FC<PageHeaderSubtitleProps> = ({
		children,
		condensed,
		}) => {
		return (
		<h2
		css={(theme) => ({
		fontSize: 16,
		color: theme.palette.text.secondary,
		fontWeight: 400,
		display: "block",
		margin: 0,
		marginTop: condensed ? 4 : 8,
		lineHeight: "140%",
		})}
		className={cn(
		"text-sm text-content-secondary font-normal block m-0 leading-snug",
		)}
		>
		{children}
		</h2>

Choose a reason for hiding this comment

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

Breaking change: The condensed prop is removed from the implementation but still in the interface and used in 3 files:

site/src/pages/TemplatePage/TemplatePageHeader.tsx (2 usages)
site/src/pages/StarterTemplatePage/StarterTemplatePageView.tsx (1 usage)

The original used condensed to control margin-top (4px vs 8px). You need to either:

Remove the prop from the interface AND update all 3 call sites, OR
Keep the prop and apply it to the Tailwind classes

If keeping the prop:


	export const PageHeaderSubtitle: FC<PageHeaderSubtitleProps> = ({
	children,
	condensed,
	}) => {
	return (
	<h2
	css={(theme) => ({
	fontSize: 16,
	color: theme.palette.text.secondary,
	fontWeight: 400,
	display: "block",
	margin: 0,
	marginTop: condensed ? 4 : 8,
	lineHeight: "140%",
	})}
	className={cn(
	"text-sm text-content-secondary font-normal block m-0 leading-snug",
	)}
	>
	{children}
	</h2>
	export const PageHeaderSubtitle: FC<PageHeaderSubtitleProps> = ({
	children,
	condensed,
	}) => {
	return (
	<h2
	className={cn(
	"text-sm text-content-secondary font-normal block m-0 leading-snug",
	)}
	>
	{children}
	</h2>
	);
	};

Choose a reason for hiding this comment

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

This was intentionally removed, its unused and not inline with the Figma.

		letterSpacing: "0.1em",
		})}
		>
		<span className="text-sm text-content-secondary font-medium uppercase tracking-widest">

Choose a reason for hiding this comment

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

Font weight change: Original was fontWeight: 600 (font-semibold), now font-medium (500). Verify this matches Figma specs.

If keeping original weight:

	<span className="text-sm text-content-secondary font-medium uppercase tracking-widest">
	<span className="text-sm text-content-secondary font-semibold uppercase tracking-widest">

Choose a reason for hiding this comment

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

This was intentional, its inline with Figma.

Comment on lines 103 to +108

		}) => {
		return (
		<p className={cn("m-0 text-content-secondary leading-relaxed", className)}>
		<p
		className={cn(
		"m-0 text-content-secondary font-medium leading-normal",
		className,

Choose a reason for hiding this comment

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

Font weight addition: Adding font-medium changes weight from 400 to 500. The line-height also changed from leading-relaxed to leading-normal. Please verify both changes match Figma.

If keeping original:

	}) => {
	return (
	<p className={cn("m-0 text-content-secondary leading-relaxed", className)}>
	<p
	className={cn(
	"m-0 text-content-secondary font-medium leading-normal",
	className,
	className={cn(
	"m-0 text-content-secondary leading-relaxed",
	className,
	)}

Choose a reason for hiding this comment

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

This was intentional, its inline with Figma.

Comment on lines 36 to 40

		<h1
		css={{
		fontSize: 24,
		fontWeight: 400,
		margin: 0,
		display: "flex",
		alignItems: "center",
		lineHeight: "140%",
		}}
		className={cn(
		"text-3xl font-semibold m-0 flex items-center leading-snug",
		)}
		>

Choose a reason for hiding this comment

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

No need for cn utility here

		<h1 className="text-3xl font-semibold m-0 flex items-center leading-snug">

Comment on lines +56 to +58

		className={cn(
		"text-sm text-content-secondary font-normal block m-0 leading-snug",
		)}