Skip to content

fix: BL-14377 move box-sizing to emotion instead of nonworking .css file#78

Merged
andrew-polk merged 1 commit intomainfrom
BL-14377_box-sizing_to_emotion
Mar 5, 2025
Merged

fix: BL-14377 move box-sizing to emotion instead of nonworking .css file#78
andrew-polk merged 1 commit intomainfrom
BL-14377_box-sizing_to_emotion

Conversation

@nabalone
Copy link
Collaborator

@nabalone nabalone commented Mar 5, 2025

This change is Reviewable

@nabalone
Copy link
Collaborator Author

nabalone commented Mar 5, 2025

I'm really not sure if this is the best way to do it. We could use mui's Baseline (https://mui.com/material-ui/react-css-baseline/?srsltid=AfmBOorq3mTrEnkHbOaidivg-_fY3eFoWvEAz31011_UCh4oWbW9Y_bg), but I'm not sure we want to do that. Seems like there should be a better way, but I can't find it. But I think this is at least in an improvement in that all the css is in emotion rather than just 1 thing being in a separate file

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, all discussions resolved


components/language-chooser/react/language-chooser-react-mui/src/demos/LanguageChooserDialog.tsx line 49 at r1 (raw file):

  const theme = useTheme();
  const dialogActionButtons = (
    <ThemeProvider theme={theme}>

just put the ThemeProvider on everything top-level for consistency

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any downside to using the baseline other than retesting to make sure we didn't break something?

If we do end up doing our own reset in emotion, is it worth creating it as a const somewhere?

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this this morning. We should try using the CSSBaseline. Note, I'm unsure of the differences between that and ScopedCssBaseline, but perhaps we want that since it isn't at the Bloom root? I'm thinking not. I.e. we probably do want CSSBaseline.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@nabalone nabalone force-pushed the BL-14377_box-sizing_to_emotion branch from daeb9c2 to 6280c3b Compare March 5, 2025 22:05
Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSSBaseline adds the css to the <head<, so I believe it can affect others of the client's components. I think we should use ScopedCssBaseline. We don't really have an app layer, Bloom just imports and uses the LanguageChooser react component

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)


components/language-chooser/react/language-chooser-react-mui/src/LanguageChooser.tsx line 297 at r2 (raw file):

      <ScopedCssBaseline // Puts box-sizing: border-box and other "normalizations" on all descendants
        css={css`
          height: 100%;

ScopedCssBaseline is a div, so I have to do this but I don't like it. Should I be taking a different approach to css height?

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)


components/language-chooser/react/language-chooser-react-mui/src/LanguageChooser.tsx line 297 at r2 (raw file):

Previously, nabalone (Noel) wrote…

ScopedCssBaseline is a div, so I have to do this but I don't like it. Should I be taking a different approach to css height?

I looked into this a little and didn't see a better alternative. We could use flex and grow, but I'm not seeing any advantage to that at this point.

@andrew-polk andrew-polk merged commit cde7dbe into main Mar 5, 2025
2 checks passed
@andrew-polk andrew-polk deleted the BL-14377_box-sizing_to_emotion branch March 5, 2025 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants