fix: update component styling to defer more to MUI theme #143
Conversation
There was a problem hiding this comment.
I wish I didn't need to make a function like this, but it really felt like it was the best way to ensure that the theme logic doesn't randomly break.
One of the problems with the base MUI theme object that Backstage gives you by default is that it's light on the number of font sizes. You generally want very few sizes for longform text, but for UI, you need more variety to have better information hierarchy
I was able to get pretty close to the old design without needing this function, but then I needed to access weird properties like theme.typography.h5.fontSize, and:
- Saying that you want your h2 to be the size of an h5 feels weird and fragile
- Realistically, I don't expect many MUI users to actually get so detailed with their theme configuration that they'll ever touch the
h5property, so the code is more likely to be out of sync, even though that defeats the point of a theme
I think it's safe to say that most users will at least look at body1 and body2, which is why I made it a bit easier to base custom sizes off of them
There was a problem hiding this comment.
I might be on the wrong page, but should we not just use h2.fontSize for h2, even if it looks wonky? Or alternatively, does this mean we should actually be using an h5 instead of an h2? My impression is that we should not try to work around MUI's problems, that is something for Backstage to sort out. 😆
| // It shouldn't be possible for these branches to run, because parent | ||
| // code should show main content instead of the form when the user is | ||
| // authenticated. Still adding them for extra assurance |
There was a problem hiding this comment.
I might be on the wrong page, but should we not just use h2.fontSize for h2, even if it looks wonky? Or alternatively, does this mean we should actually be using an h5 instead of an h2? My impression is that we should not try to work around MUI's problems, that is something for Backstage to sort out. 😆
|
@code-asher Yeah, that's fair. Supposedly we're going to have a call with the Spotify team soon, so I might park this until I have a chance to talk to them about how they approach setting up the theme At the end of the day, these changes are in service of making sure the plugin doesn't look wonky, but I just wish that I didn't have to worry about that, and that things just worked. |
|
Yeah, that makes sense. Do we have external pressure to make it look better? If not I say we embrace the wonk! |
| return `${baseSize * scale}px`; | ||
| } | ||
|
|
||
| const sizeRe = /^\s*(?<value>\d+(?:\.\d+))?s*(?<unit>px|r?em)\s*$/i; |
There was a problem hiding this comment.
I'm taking your regular expressions away it's for your own good.
Part 1 of making sure that the plugins are ready for Spotify Portal (https://github.com/coder/customers/issues/676)
Changes made
Notes
line-height. I wish I could remove them, but they are there intentionally.