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

[material-ui][styles] convert createPalette code to typescript #44173

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

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Oct 21, 2024

Was going through this PR comments and this comment was the motivation to open the PR

@sai6855 sai6855 marked this pull request as draft October 21, 2024 11:28
@mui-bot
Copy link

mui-bot commented Oct 21, 2024

Netlify deploy preview

https://deploy-preview-44173--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 07f8d62

@@ -1,145 +0,0 @@
export type PaletteMode = 'light' | 'dark';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved code in this file to createPalette.ts

@sai6855 sai6855 marked this pull request as ready for review October 21, 2024 12:47
@sai6855 sai6855 requested a review from Janpot October 21, 2024 12:48
@sai6855 sai6855 added typescript package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. package: material-ui Specific to @mui/material and removed package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Oct 21, 2024
A700: string;
}

export {};
Copy link
Member

Choose a reason for hiding this comment

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

Does this have a purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure, i just copy pasted entire code from createPalette.d.ts to this file.

lightShade = 300,
darkShade = 700,
}: PaletteAugmentColorOptions): PaletteColor => {
const colorInput = { ...color } as PaletteColor;
Copy link
Member

Choose a reason for hiding this comment

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

There are 4 or 5 places with type casting, especially this one and the createPalette return type. Can we try avoiding type casting without breaking public types?

Copy link
Contributor Author

@sai6855 sai6855 Oct 25, 2024

Choose a reason for hiding this comment

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

Yeah i agree, there are lot of type castings, initially i tried to avoid castings but it seemed impossible without breaking public api's.

For example: let's take this type here. I applied casting to shade type here to make sure type of shade is one of keys in intent. So to remove this casting, we need to change type of shade to number | keyof PaletteColor from number | string.

So if we change shade type, we need to change type of lightShade and darkShade from number | string to number | keyof PaletteColor as both are passed as shade here and this results in changing PaletteAugmentColorOptions type which is a public api.

Let me know, if you need explainations for other type castings

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.

3 participants