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

[system] Box accept any value in TypeScript #15451

Closed
2 tasks done
Jack-Works opened this issue Apr 23, 2019 · 9 comments · Fixed by #23599
Closed
2 tasks done

[system] Box accept any value in TypeScript #15451

Jack-Works opened this issue Apr 23, 2019 · 9 comments · Fixed by #23599
Labels
new feature New feature or request package: system Specific to @mui/system typescript

Comments

@Jack-Works
Copy link
Contributor

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Demo in the document

Let's see the demo in the https://material-ui.com/system/basics/

<Box
  color="primary.main"
  bgcolor="background.paper"
  fontFamily="h6.fontFamily"
  fontSize={{ xs: 'h6.fontSize', sm: 'h4.fontSize', md: 'h3.fontSize' } }
  p={{ xs: 2, sm: 3, md: 4} }
>
  @material-ui/system
</Box>

The content of color bgcolor font-Family` ... is not type-friendly.

Now we can use variant (typed as 'outlined' | 'contained' | ...etc ) or color (typed as 'primary' | 'secondary' | ...etc). In the case of Box, it just like you want to type it as "outline.primary". You need to write all the combinations to get the correct type.

Consider a better API like:

<Box
  color={['primary', 'main']}
  bgcolor={['background', 'paper']}
  fontFamily={['h6', 'fontFamily']}
  p={{ xs: 2, sm: 3, md: 4} }
>
  @material-ui/system
</Box>

Then we can type Box as

interface BoxProps {
    color: ['primary' | 'secondary', 'main' | '...'] // Type it as a tuple!
    fontFamily: ['h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6', keyof AllCSSRules] //🙌
    // ...
}

And this doesn't need to break the current behavior. Just .join('.') and ['primary', 'main'] becomes "primary.main"

@Losses
Copy link
Contributor

Losses commented Apr 23, 2019

Something like

<Box
  color={{muiStyles.primary.main}}
  bgcolor={{muiStyles.background.paper}}
  ...
>
  @material-ui/system
</Box>

Is also better than demo in the document.

@eps1lon eps1lon added new feature New feature or request package: system Specific to @mui/system typescript labels Apr 23, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 23, 2019

@Jack-Works On a very basic level this is a better solution. String paths usually create a refactoring hazard. But what happens with the types in custom themes? We would need type helpers that take a Theme and then infer the possible keys at every level.

This is also something to keep in mind when we introduce a dynamic color prop.

@Losses Where does muiStyles come from?

@Losses
Copy link
Contributor

Losses commented Apr 23, 2019

@eps1lon Some hypothetical variable

@eps1lon
Copy link
Member

eps1lon commented Apr 23, 2019

@eps1lon Some hypothetical variable

This is not actionable for us. If you hide implementation details then the demo is always better than a demo with implementation details. But we want to showcase actual usage where your code doesn't apply.

@Jack-Works
Copy link
Contributor Author

To support both approach, use string | [ tuple ].
Don't use string | [ "Val"| string ], this will omit "Val" and result in string | [ string ] and get untyped again

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 1, 2019

@eps1lon Aren't we in much better shape now?

@Jack-Works
Copy link
Contributor Author

I have used box days ago. I feel its better typed now.

@Jack-Works
Copy link
Contributor Author

But only some attributes get typed I thought. color and bg.color seems keep the same

@eps1lon eps1lon changed the title material-ui/system Design of box is very unfriendly with TypeScript material-ui/system Box accept any value in TypeScript Jun 24, 2019
@akomm
Copy link
Contributor

akomm commented Jul 16, 2019

@eps1lon I am working on exactly that. And actually have a semi-working solution. Semi, because it works - but not with generic theme mapping functions, which you need to resolve something like this: ["primary", "main"]. Here a sandbox of it (WIP): https://codesandbox.io/s/epic-pascal-bi04v

It would work perfectly well with custom themes, it even make the whole process precise, because styles in the demo specify only exactly what they need and whether they need theme at all. There is a custom style function, not from system, as it is currently not in a way I would like it to be, but I try to work on the issues and make proposals, one after another, currently there is a breaking bug in jss I have PR running, after which I can continue with the refined PRs from the draft you might recall.

Instead of themeKey, you have a themeMap function which projects values from prop/theme to value(s). Advantage of this method is also that you can can not only map a prop value to theme value, but also use additional theme values, which maybe are not directly tied to the prop.

The downside of passing array literals is the performance.

Not part of the demo, just a naive thought:

<Component bgcolor={color("primary", "main")} textcolor={color("secondary", "main")} />

Where bgcolor/textcolor simply takes a css color and not theme key. This has obviously some cost in term of design then... and some benefits, like easier typing, better performance.

However:

  • Now bgcolor must be connected to a theme, and this theme must be "adjustable"
  • You want a way to have a "theme" as object, and not 1000 functions spread all over the place. Need some design how this can happen fluently. Hoc that creates such functions from theme for example. This would the theme depending on how the hoc is made immutable then...

Note:
Such a semantic, like primary.main has some domain to it, so it has some ties to a theme its based on though. So I think if mui provides a default theme or structure needed for its core components, it ships such functions tied to a theme that has required shape and data types, even if you customize the theme, this shape is required by the components at least. And if use customizes and adds his own style(s), he can also write such transformation functions for it specific to his theme requirements.

@oliviertassinari oliviertassinari changed the title material-ui/system Box accept any value in TypeScript [system] Box accept any value in TypeScript Oct 20, 2020
This was referenced Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants