chore: migrate appearanceform to Tailwind and shadcn #20204
Conversation
aslilac
left a comment
There was a problem hiding this comment.
I was kinda dreading this one. but it looks great! thanks!
| import type { Interpolation } from "@emotion/react"; | ||
| import CircularProgress from "@mui/material/CircularProgress"; | ||
| import FormControl from "@mui/material/FormControl"; | ||
| import FormControlLabel from "@mui/material/FormControlLabel"; | ||
| import Radio from "@mui/material/Radio"; | ||
| import RadioGroup from "@mui/material/RadioGroup"; | ||
| import { visuallyHidden } from "@mui/utils"; |
There was a problem hiding this comment.
that's a satisfying chunk of imports to watch vanish
buenos-nachos
left a comment
There was a problem hiding this comment.
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> | ||
| <div css={{ marginBottom: 48 }}></div> | ||
| <div className="mb-12" /> |
There was a problem hiding this comment.
Do we still need this empty div? Does the Section component not support className?
There was a problem hiding this comment.
Nice catch, moved the mb-12 to Section as a className
| </RadioGroup> | ||
| </FormControl> | ||
| <RadioGroup | ||
| aria-labelledby="fonts-radio-buttons-group-label" |
There was a problem hiding this comment.
This looks like invalid HTML to me. If we have aria-labelledby, we need another element in the HTML that has (1) an id attribute with a matching value and (2) some kind of labeling text
There was a problem hiding this comment.
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.
Do we need this filter call, or can we trust the backend-generated types to not include a zero value?
There was a problem hiding this comment.
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.
No description provided.