- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): standardize headers for Admin Settings page#16911
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| <StackalignItems="baseline"direction="row"justifyContent="space-between"> | ||
| <divcss={{maxWidth:420,marginBottom:24}}> | ||
| <Stackdirection="row"spacing={1}alignItems="center"> | ||
| <h1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was a big accessibility/HTML validity issue. We were using this component multiple times on the same page, but only oneh1 is allowed to be on a page, total
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is why I decoupled thesecondary 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
| <divclassName="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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The different leading values for the header and description are intentional – leading needs to go down the larger your text is
| <> | ||
| Notifications | ||
| <spancss={{position:"relative",top:"-6px"}}> | ||
| <spancss={{position:"relative",top:"2px"}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Needed to adjust this slightly to account for the updated text leading
BrunoQuaresmaApr 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| "& code":{ | ||
| background:theme.palette.divider, | ||
| fontSize:12, | ||
| padding:"2px 4px", | ||
| color:theme.palette.text.primary, | ||
| borderRadius:2, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This code seemed to be part of a previous version of the component. None of the styling ever applied to anything
| }=useProxy(); | ||
| return( | ||
| <Section |
ParkreinerMar 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was deleted because the header is now located in the view, which I think is how it should've always been
@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 |
| import{twMerge}from"tailwind-merge"; | ||
| interfaceHeaderProps{ | ||
| typeHeaderHierarchy="primary"|"secondary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Made it like this so that we can add additional variants over time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For creating variants, I would recommend you to useclass-variance-authority package. You can see how it is used in theButton component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| import{twMerge}from"tailwind-merge"; | ||
| interfaceHeaderProps{ | ||
| typeHeaderHierarchy="primary"|"secondary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For creating variants, I would recommend you to useclass-variance-authority package. You can see how it is used in theButton component.
| })=>{ | ||
| consttheme=useTheme(); | ||
| constHeaderTitle=titleHeaderLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of doing this, we have access to theSlot component from Radix. I think it is a better tool for this kinda of "polymorphic" component.
ParkreinerMar 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| description?:ReactNode; | ||
| secondary?:boolean; | ||
| titleVisualHierarchy?:HeaderHierarchy; | ||
| titleHeaderLevel?:HeaderLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know it is how we did before but since we are touching this, could we also improve the abstraction? I see something like:
<SettingsHeaderactions={[<SeeDocsButton/>]}><SettingsHeaderTitle>Mytitle</SettingsHeaderTitle><SettingsHeaderSubtitle>Some text hear</SettingsHeaderSubtitle></SettingsHeader>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we add a storybook for it covering the new variants?
Parkreiner commentedMar 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 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 |
| // 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added a comment to call out why the code isn't usingSlot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the snapshots look good, go for it!
fcac4ab intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
No issue to link – it's just a small thing that has been on my radar for a bit
Changes made
SettingHeadercomponentSettingHeaderto be based on Radix and Tailwind, rather than Emotion/MUISettingHeaderto help resolve issues with the component creating invalid HTMLSettingHeaderto have better out-of-the-box accessibilitySettingHeaderSettingsHeaderSettingsHeaderNotes
SettingHeaderyet. 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 leastlook like the other headers, but it'd be nice if we could centralize everything in a followup PRScreenshots
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
