Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MUI theme decorator and Typography styles #1494

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

edburyenegren-okta
Copy link
Contributor

@edburyenegren-okta edburyenegren-okta commented Jun 8, 2022

Description

Adds a MUI Theme decorator for use until we can update Storybook.

Updates the Primary.Main color to the appropriate value.

Adds component overrides and a starting point for TS augmentations.

@@ -67,13 +69,7 @@ export const Themed = (
},
});

return (
<MuiThemeProvider theme={theme}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this breaks the display in some ways. Not sure what is going on here:

Screen Shot 2022-06-08 at 1 59 02 PM

Our blue is being applied, but not our typeface. This might be expected because we don't have a font-face declaration inside the theme yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the theme to use a different primary main color to check that the decorator works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The screenshot here is reflecting the correct color from the main director, but the "Themed" example is broken without the wrappers.

I've added the wrappers back to protect the previous work, since that's not really the point of this PR. Normal, non-theme override stories should work fine without the wrappers.

Copy link
Contributor

@sherizada-okta sherizada-okta Jun 8, 2022

Choose a reason for hiding this comment

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

So in order to get the decorator to use the custom theme for this story, we can do this:

# packages/odyssey-storybook/src/components/PasswordInput/PasswordInput.stories.tsx
Themed.args = {
  theme: createTheme({
    palette: {
      mode: "light",
      primary: {
        main: "#3fb466",
      },
    },
  })
};

# packages/odyssey-storybook/.storybook/components/MuiThemeDecorator.tsx
export const MuiThemeDecorator: DecoratorFn = (Story, {args: {theme}) => (
  <MuiThemeProvider theme={theme}>
    <ThemeProvider theme={theme}>
      <Story />
    </ThemeProvider>
  </MuiThemeProvider>
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like my previous comment doesn't work for when changing the color via Controls

//overline: false;
//}
//}
declare module "@mui/material/styles" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sherizada-okta I'm not sure where these should live. I have them alongside the theme overrides right now for ease of comparison, but we should be exporting them somewhere.

Copy link
Contributor

@sherizada-okta sherizada-okta Jun 8, 2022

Choose a reason for hiding this comment

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

Looks like the declares should be in separate files following the pattern types.d.ts (https://www.typescriptlang.org/docs/handbook/declaration-files/introduction.html).

Just tried this and now the import from "@mui/material/styles" in theme.ts shows this error:
Module '"@mui/material/styles"' has no exported member 'createTheme'

Copy link
Contributor

Choose a reason for hiding this comment

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

And then inside packages/odyssey-react-mui/tsconfig.json we'll need to add:

  "include": [
      "@okta/odyssey-react-mui/themes/odyssey/types.d.ts"
    ]

Copy link
Contributor Author

@edburyenegren-okta edburyenegren-okta Jun 8, 2022

Choose a reason for hiding this comment

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

I think we get the createTheme error because the theme object can't be parsed without the type augmentations:

src/themes/odyssey/typography.ts(80,3): error TS2322: Type '{ htmlFontSize: number; fontFamily: "'Public Sans', '-apple-system', 'BlinkMacSystemFont', 'Segoe UI', 'Roboto', 'Oxygen-Sans', 'Ubuntu', 'Cantarell', 'Helvetica Neue', 'Noto Sans Arabic', sans-serif"; ... 18 more ...; overline: undefined; }' is not assignable to type 'TypographyOptions | ((palette: Palette) => TypographyOptions) | undefined'.
  Object literal may only specify known properties, and 'body' does not exist in type 'TypographyOptions | ((palette: Palette) => TypographyOptions)'

This occurs when types.d.ts is present, whether it's included in our tsconfig.json or not -- but not when the declarations live alongside the theme object, which is odd.


## Type styles

<MuiThemeProvider theme={theme}>
Copy link
Contributor Author

@edburyenegren-okta edburyenegren-okta Jun 8, 2022

Choose a reason for hiding this comment

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

The decorator only applies to stories, so non-story MUI examples still need a wrapper. Once I have an example for story'ing MUI components directly, I can convert these examples to stories.

@edburyenegren-okta edburyenegren-okta changed the title feat(odyssey-storybook): add MUI theme decorator, update primary main Add MUI theme decorator and Typography styles Jun 8, 2022
@@ -43,9 +43,9 @@ export const palette: ThemeOptions["palette"] = {
contrastText: "#1d1d21",
},
info: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes info match primary without removing the distinction.

MuiTypography: {
styleOverrides: {
paragraph: {
marginBottom: "1.14285714rem",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we getting these values from ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the values from our design tokens. We don't have an integration with StyleDictionary yet, so we can't pull them in from there. I believe @sherizada-okta is going to test importing them the MUI way via theme.spacing(2). Once we have a solution that works, I'll swap all of these magic numbers over to variables (same for our colors, line-heights, etc).

@edburyenegren-okta edburyenegren-okta merged commit 36da1c7 into develop Jun 9, 2022
@edburyenegren-okta edburyenegren-okta deleted the ee/mui-typography branch June 9, 2022 22:26
interface TypographyPropsVariantOverrides {
body1: false;
body2: false;
body: true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you introduced a new variant body, will that need a variant mapping?

const theme = createTheme({
  components: {
    MuiTypography: {
      defaultProps: {
        variantMapping: {
          h1: 'h2',
          h2: 'h2',
          h3: 'h2',
          h4: 'h2',
          h5: 'h2',
          h6: 'h2',
          subtitle1: 'h2',
          subtitle2: 'h2',
          body1: 'span',
          body2: 'span',
        },
      },
    },
  },
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without one, it defaults to span which felt appropriate for body. I can def add this if we want to be explicit tho!

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.

4 participants