chore: use emotion for styling (pt. 2) #9951
Conversation
author Kayla Washburn <mckayla@hey.com> 1696014497 -0600 committer McKayla Washburn <mckayla@hey.com> 1696014979 +0000 parent 4da1223 author Kayla Washburn <mckayla@hey.com> 1696014497 -0600 committer McKayla Washburn <mckayla@hey.com> 1696014955 +0000 parent 4da1223 author Kayla Washburn <mckayla@hey.com> 1696014497 -0600 committer McKayla Washburn <mckayla@hey.com> 1696014933 +0000 parent 4da1223 author Kayla Washburn <mckayla@hey.com> 1696014497 -0600 committer McKayla Washburn <mckayla@hey.com> 1696014828 +0000 refactor: start using emotion for styling (#9909) get emotional apologize to the json gods `make gen` fix types this is killing me fancy `Theme` shenanigans >:( caught some mistakes do a couple more (to fix storybook) 🤬 fix linting
32e5596 to
23404c6
Compare
| const { isCopied, copy: copyToClipboard } = useClipboard(text); | ||
|
|
||
| const fileCopyIconStyles = css` |
There was a problem hiding this comment.
Thoughts on these style rules being at the top of the component vs at the bottom?
There was a problem hiding this comment.
I think I prefer to define things top to bottom, but I'm fine with anything.
Another question, though: is there any difference between having the css tag function defined inside the component vs outside?
There was a problem hiding this comment.
I've always put stateful logic (hooks, contexts, queries) at the top of the file while moving CSS to the bottom, but I don't have a strong opinion, just a habit. I'm happy with either. Would be nice to stay consistent either way we go.
There was a problem hiding this comment.
yeah, I like top-to-bottom. my instinct is always to scroll up for stuff like this, and I'm always a little sad when I realize I was supposed to scroll down. :p
There was a problem hiding this comment.
although that's probably less of an issue here than it was with makeStyles, because "go to definition" will actually work now 🙃
| width: theme.spacing(6), | ||
| height: theme.spacing(6), | ||
| fontSize: theme.spacing(3), | ||
| }), | ||
| }; | ||
| } satisfies Record<string, Interpolation<Theme>>; |
There was a problem hiding this comment.
Could the Record be updated to be indexed by type Exclude<AvatarProps["size"], undefined>?
There was a problem hiding this comment.
Though maybe it'd be better to life the size prop into a separate type, and just reference it in AvatarProps and sizeStyles
There was a problem hiding this comment.
that's what is cool about satisfies, it doesn't widen the type, it makes sure that T extends Record<...>. if you added an incorrect key here, it would still cause a type error at the lookup, because the keys are still literal types.
There was a problem hiding this comment.
that's what is cool about satisfies, it doesn't widen the type, it makes sure that T extends Record<...>. if you added an incorrect key here, it would still cause a type error at the lookup, because TS still knows the actual keys the object has.
There was a problem hiding this comment.
So, you're right that it doesn't widen the type, but I guess the way I look at it is that right now, we have a narrow type, but wide type-checking, when ideally both would be as narrow as possible.
So for this example specifically, there's nothing requiring that all sizes be present in the record – the key for type-checking is any arbitrary string, so TypeScript has no ability to detect typos in key names or forgotten keys. All it knows is that there should be some number of strings in there (including zero)
Like, if we decide to add a "lg" size down the line, there aren't any protections to make sure we don't forget to add a matching style. But with the record being keyed by all members of the union, TypeScript will complain if something doesn't line up
There was a problem hiding this comment.
there are! the lookup will fail, we don't need a more stringent satisfies to catch that. it'd just make this type harder to write.
There was a problem hiding this comment.
Oh wait, yeah, you're right. I was worried because we don't have the noUncheckedIndexedAccess compiler setting turned on, which can cause issues with trying to read properties from a record, but because it technically isn't a record, and just has the type-checking of one, it's safe.
So we will get errors when trying to consume a property that doesn't exist, instead of weird false positives
| const { isCopied, copy: copyToClipboard } = useClipboard(text); | ||
|
|
||
| const fileCopyIconStyles = css` |
There was a problem hiding this comment.
I think I prefer to define things top to bottom, but I'm fine with anything.
Another question, though: is there any difference between having the css tag function defined inside the component vs outside?
Part of #9924
Following #9909
From starting with 157 usages of makeStyles across 154 files, this changes brings us down to 132 usages across 129 files.