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] Use the CustomSystemProps interface in Box Props #35593

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

IgnusG
Copy link

@IgnusG IgnusG commented Dec 23, 2022

Resolves #35565

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Should we also add a test that the custom system prop provided directly to the system components (Box, Typography, etc) applies the styling properly (probably from the theme?) ?

packages/mui-system/src/Box/Box.spec.tsx Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: system Specific to @mui/system labels Dec 24, 2022
@IgnusG
Copy link
Author

IgnusG commented Jan 13, 2023

Should we also add a test that the custom system prop provided directly to the system components (Box, Typography, etc) applies the styling properly (probably from the theme?) ?

I think this might be out of scope for this PR. It's up to the user to make sure these props are handled correctly by I think using this experimental API for handling sx which system props use internally to decide what to do with props that are custom?

Without using this API it can still be useful to use CustomSystemProps for restricting the values on existing custom props, eg. forcing fontWeight to only accept values medium and bold (which map to fontWeightMedium and fontWeightBold in the theme) instead of allowing anything including custom font size numbers (200, 300 etc.)

@ZeeshanTamboli
Copy link
Member

Should we also add a test that the custom system prop provided directly to the system components (Box, Typography, etc) applies the styling properly (probably from the theme?) ?

I think this might be out of scope for this PR. It's up to the user to make sure these props are handled correctly by I think using this experimental API for handling sx which system props use internally to decide what to do with props that are custom?

Without using this API it can still be useful to use CustomSystemProps for restricting the values on existing custom props, eg. forcing fontWeight to only accept values medium and bold (which map to fontWeightMedium and fontWeightBold in the theme) instead of allowing anything including custom font size numbers (200, 300 etc.)

@mnajdova would know better.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jan 18, 2023

Also please update with latest master branch to fix the CI.

@JoseGalRe
Copy link

Hi, there any new about this PR?

@mui-bot
Copy link

mui-bot commented May 2, 2023

Netlify deploy preview

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 720b933

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

The test_types CI job is failing. It is because the types are mismatched between sx prop aliases mt, pt etc. in StandardCssProperties and AliasesCSSProperties. Let's remove it. The following diff should work:

--- a/packages/mui-system/src/Box/Box.d.ts
+++ b/packages/mui-system/src/Box/Box.d.ts
@@ -6,7 +6,6 @@ import {
   AllSystemCSSProperties,
   ResponsiveStyleValue,
   OverwriteCSSProperties,
-  AliasesCSSProperties,
 } from '../styleFunctionSx';

 export type PropsFor<SomeStyleFunction> = SomeStyleFunction extends StyleFunction<infer Props>
@@ -141,7 +140,7 @@ export type ComposedStyleFunction<T extends Array<StyleFunction<any>>> = StyleFu
   ComposedOwnerState<T>
 > & { filterProps: string[] };

-export interface CustomSystemProps extends AliasesCSSProperties, OverwriteCSSProperties {}
+export interface CustomSystemProps extends OverwriteCSSProperties {}

 export type SimpleSystemKeys = keyof PropsFor<
   ComposedStyleFunction<

Without using this API it can still be useful to use CustomSystemProps for restricting the values on existing custom props, eg. forcing fontWeight to only accept values medium and bold (which map to fontWeightMedium and fontWeightBold in the theme) instead of allowing anything including custom font size numbers (200, 300 etc.)

This sounds like a good reason to accept this PR. I would also like it if you added the docs about restricting the values on existing properties under https://mui.com/system/getting-started/the-sx-prop/#typescript-usage with an example.

@IgnusG
Copy link
Author

IgnusG commented May 2, 2023

@ZeeshanTamboli I will include the requested changes 👍

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

I ran the CI and the TypeScript tests failed with the following error:

Expression produces a union type that is too complex to represent.ts(2590)

for

    <BoxStyled error={error} success={success} className={className} {...others}>
      <Typography color="inherit">{children}</Typography>
    </BoxStyled>

It could be related to #34068 or it could be a TypeScript problem - microsoft/TypeScript#52459.

Or perhaps it is occurring because the sx prop type is too complex and TypeScript cannot handle it with long unions. We would like to simplify it first. See the discussion in #19113.

If you have a solution for this, please go ahead. Otherwise, we will have to hold off this PR.

@ZeeshanTamboli ZeeshanTamboli added the on hold There is a blocker, we need to wait label May 18, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work on hold There is a blocker, we need to wait package: system Specific to @mui/system PR: out-of-date The pull request has merge conflicts and can't be merged typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CustomSystemProps is not used in SystemProps anymore
5 participants