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

[TypeScript] getCssVar autocomplete for Material UI #33464

Merged
merged 8 commits into from
Jul 11, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 11, 2022

@siriwatknp siriwatknp added typescript package: material-ui Specific to @mui/material labels Jul 11, 2022
@mui-bot
Copy link

mui-bot commented Jul 11, 2022

No bundle size changes

Generated by 🚫 dangerJS against dbc7ed0

@siriwatknp siriwatknp requested a review from mnajdova July 11, 2022 04:33
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks great, tested here - https://codesandbox.io/s/basicbuttons-demo-material-ui-forked-u9f18y?file=/demo.tsx

Would be great if we can add a *.spec.ts where we test that we can invoke the getCssVar with valid theme value and with custom var too. We can even go one step further and test that module augmentation will be affected too.

@siriwatknp
Copy link
Member Author

Looks great, tested here - https://codesandbox.io/s/basicbuttons-demo-material-ui-forked-u9f18y?file=/demo.tsx

Would be great if we can add a *.spec.ts where we test that we can invoke the getCssVar with valid theme value and with custom var too. We can even go one step further and test that module augmentation will be affected too.

Added! I changed my mind to make it strict first (only work with the values inside the theme).

Developers can augment by:

declare module "@mui/material/styles" {
  interface ThemeCssVarOverrides {
    customToken: true
  }
}

theme.getCssVar('customToken') // 🎉

or cast with any for unknown variable

theme.getCssVar('palette-primary-main', 'var(--custom-token)' as any)

@siriwatknp siriwatknp merged commit f427ef9 into mui:master Jul 11, 2022
? V extends string | number
? keyof T
: keyof V extends string | number
? `${keyof T}-${ConcatDeep<Split<V>>}`
Copy link

Choose a reason for hiding this comment

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

I'm not at all a Typescript expert, but doesn't seem to work with Typescript 3.x and the docs say that this is Typescript 3.5 compatible.

This isn't the only place, but most are in here. @mui/material 5.9.1, fails latest TS 3.x, fine latest TS 4.x.

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:304:9 - error TS1110: Type expected.

304       ? `${keyof T}-${ConcatDeep<Split<V>>}`
            ~~~

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:304:18 - error TS1005: '}' expected.

304       ? `${keyof T}-${ConcatDeep<Split<V>>}`
                     ~

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:304:19 - error TS1128: Declaration or statement expected.

304       ? `${keyof T}-${ConcatDeep<Split<V>>}`
                      ~

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:304:22 - error TS1005: ';' expected.

304       ? `${keyof T}-${ConcatDeep<Split<V>>}`
                         ~

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:304:43 - error TS1005: '(' expected.

304       ? `${keyof T}-${ConcatDeep<Split<V>>}`
                                              ~

node_modules/@mui/material/styles/experimental_extendTheme.d.ts:398:1 - error TS1160: Unterminated template literal.

398 
    

node_modules/@mui/system/Unstable_Grid/GridProps.d.ts:95:90 - error TS1005: '>' expected.

95 declare type CustomBreakpoints = Partial<Record<Breakpoint, boolean | GridSize> & Record<`${Breakpoint}Offset`, GridSize>>;
                                                                                            ~~~

node_modules/@mui/system/Unstable_Grid/GridProps.d.ts:95:123 - error TS1109: Expression expected.

95 declare type CustomBreakpoints = Partial<Record<Breakpoint, boolean | GridSize> & Record<`${Breakpoint}Offset`, GridSize>>;
                                                                                                                             ~


Found 8 errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it break even if you don't use those APIs? If yes, please open an issue.

Copy link

@elyobo elyobo Jul 22, 2022

Choose a reason for hiding this comment

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

Yeah, I'm not using extendTheme but tsc -noEmit was causing the above output with a couple of 3.x versions. It's also not a problem for me in the particular project I ran into with, I can just upgrade the typescript version without pain (and have done so...).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants