- Notifications
You must be signed in to change notification settings - Fork1.1k
chore: migrate appearanceform to Tailwind and shadcn#20204
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
aslilac left a comment
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 was kinda dreading this one. but it looks great! thanks!
| importtype{Interpolation}from"@emotion/react"; | ||
| importCircularProgressfrom"@mui/material/CircularProgress"; | ||
| importFormControlfrom"@mui/material/FormControl"; | ||
| importFormControlLabelfrom"@mui/material/FormControlLabel"; | ||
| importRadiofrom"@mui/material/Radio"; | ||
| importRadioGroupfrom"@mui/material/RadioGroup"; | ||
| import{visuallyHidden}from"@mui/utils"; |
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.
that's a satisfying chunk of imports to watch vanish
buenos-nachos left a comment
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.
Your changes look good to me. Just wanted to flag a couple of spots in the code that I think could be cleaned up while we're touching the file
| </div> | ||
| </Section> | ||
| <divcss={{marginBottom:48}}></div> | ||
| <divclassName="mb-12"/> |
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.
Do we still need this empty div? Does theSection component not supportclassName?
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.
Nice catch, moved the mb-12 to Section as a className
| </RadioGroup> | ||
| </FormControl> | ||
| <RadioGroup | ||
| aria-labelledby="fonts-radio-buttons-group-label" |
buenos-nachosOct 8, 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 looks like invalid HTML to me. If we havearia-labelledby, we need another element in the HTML that has (1) anid attribute with a matching value and (2) some kind of labeling text
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 the id to the section title "Terminal Font" span
| onChangeTerminalFont(toTerminalFontName(value)) | ||
| } | ||
| > | ||
| {TerminalFontNames.filter((name)=>name!=="").map((name)=>( |
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.
Do we need thisfilter call, or can we trust the backend-generated types to not include a zero value?
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.
It looks like TerminalFontName in typesGenerated comes from codersdk/users.go with an empty string option. So it seems like we need to filter out the empty string if we don't want to display that.
9935da8 intomainUh oh!
There was an error while loading.Please reload this page.
No description provided.