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

Theme type is missing the shadows and typography properties #219

Open
illusionalsagacity opened this issue Feb 20, 2025 · 3 comments
Open

Comments

@illusionalsagacity
Copy link
Contributor

type t_theme = {
breakpoints: t_breakpoints,
components: Overrides.t,
direction: string,
palette: t_palette,
shape: t_shape,
spacing: int => int,
transitions: t_transitions,
mixins: mixins,
zIndex: t_zIndex,
}

see:
https://github.com/mui/material-ui/blob/v5.x/packages/mui-material/src/styles/createTheme.d.ts#L28-L36

https://github.com/mui/material-ui/blob/v5.x/packages/mui-material/src/styles/createTypography.d.ts#L66

https://github.com/mui/material-ui/blob/v5.x/packages/mui-material/src/styles/shadows.d.ts#L1

export interface FontStyle {
  fontFamily: React.CSSProperties['fontFamily'];
  fontSize: number;
  fontWeightLight: React.CSSProperties['fontWeight'];
  fontWeightRegular: React.CSSProperties['fontWeight'];
  fontWeightMedium: React.CSSProperties['fontWeight'];
  fontWeightBold: React.CSSProperties['fontWeight'];
  htmlFontSize: number;
}

So this probably should only support the default variants considering all the other Typography components would need to be altered, and at that point I'd imagine the consumer would just create their own bindings for each instead. Here's some partials for the create theme:

type t_fontStyle = {
  ...ReactDOM.Style.t,
  fontFamily?: string,
  fontSize?: string,
  fontWeightLight?: string;
  fontWeightRegular?: string;
  fontWeightMedium?: string;
  fontWeightBold?: string;
  htmlFontSize?: number;
}

// this shouldn't be partial for the theme type returned by useTheme for example
type t_typography = {
  allVariants?: t_fontStyle,
  h1?: t_fontStyle,
  h2?: t_fontStyle,
  h3?: t_fontStyle,
  h4?: t_fontStyle,
  h5?: t_fontStyle,
  h6?: t_fontStyle,
  subtitle1?: t_fontStyle,
  subtitle2?: t_fontStyle,
  body1?: t_fontStyle,
  body2?: t_fontStyle,
  caption?: t_fontStyle,
  button?: t_fontStyle,
  overline?: t_fontStyle,
}

Shadows probably should be typed as an array instead of a tuple because Rescript doesn't allow indexed tuple access but Typescript does

@fhammerschmidt
Copy link
Member

I basically inherited this part from the old bindings and never used that myself so feel free to adapt anything you like.

@fhammerschmidt
Copy link
Member

Also great that you clean up and extend everything now. I was considering creating a v6 branch already, but I think that can wait a bit longer until all remaining issues are fixed. v6 migration should not be so bad from what I gathered.

@illusionalsagacity
Copy link
Contributor Author

Also great that you clean up and extend everything now. I was considering creating a v6 branch already, but I think that can wait a bit longer until all remaining issues are fixed. v6 migration should not be so bad from what I gathered.

We are actually using these bindings against v6 so yeah it should not be so bad at all. I'm definitely happy to contribute for v6 stuff.

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

No branches or pull requests

2 participants