Conversation
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
![coder-tasks[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/2652121?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Reviewed heading sizing unification changes. Found 4 issues that need attention:
- Breaking change:
condensedprop removed from implementation but still used in 3 files - Duplicate class:
gap-8appears twice in PageHeader - Invalid flex value:
flex-startshould beitems-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.
|
|
||
| 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> |
There was a problem hiding this comment.
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> | |
| ); | |
| }; |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
This was intentional, its inline with Figma.
| }) => { | ||
| 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, |
There was a problem hiding this comment.
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, | |
| )} |
There was a problem hiding this comment.
This was intentional, its inline with Figma.
| <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", | ||
| )} | ||
| > |
There was a problem hiding this comment.
No need for cn utility here
<h1 className="text-3xl font-semibold m-0 flex items-center leading-snug">
| className={cn( | ||
| "text-sm text-content-secondary font-normal block m-0 leading-snug", | ||
| )} |
2 participants
This pull-request addresses heading sizing inline with Figma. This means that our headings are all uniformly
font-weightandfont-size. Furthermore, we've dropped thefont-sizeof the descriptions below the headings.Comparison
This one is harder to see, but its mild spacing resolution 🙂