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

fix: properly filter palettes #42698

Closed
wants to merge 1 commit into from

Conversation

CGNonofr
Copy link
Contributor

@CGNonofr CGNonofr commented Jun 20, 2024

If there is a palette with a main non-string field, it crashes

@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Jun 21, 2024
@zannager zannager requested a review from DiegoAndai June 21, 2024 14:54
@DiegoAndai DiegoAndai requested a review from siriwatknp June 26, 2024 20:23
@DiegoAndai
Copy link
Member

Hi @CGNonofr, thanks for the report.

What value are you providing to the main field? It should be a string, it's types as such: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/styles/createPalette.d.ts#L44

@CGNonofr
Copy link
Contributor Author

Hi @CGNonofr, thanks for the report.

What value are you providing to the main field? It should be a string, it's types as such: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/styles/createPalette.d.ts#L44

PaletteColorOptions is only used for the primary, secondary, error, warning, info and success.

The problem is it is looping of every palettes keys, not only those corresponding to a PaletteColorOptions.

This is not a problem for other predefined fields because none has a main property, but we added a custom palette field with a main field of type object.

@DiegoAndai
Copy link
Member

This is not a problem for other predefined fields because none has a main property, but we added a custom palette field with a main field of type object.

I see, @siriwatknp what do you think about this use case?

@siriwatknp
Copy link
Member

This is not a problem for other predefined fields because none has a main property, but we added a custom palette field with a main field of type object.

I see, @siriwatknp what do you think about this use case?

I think it's okay to apply this change, the performance should not be different from the existing condition. But I think it should apply to all components.

@DiegoAndai
Copy link
Member

@siriwatknp the logic on the filter is getting more complex, should we extract a utility for it so it can be shared throughout the components?

Also, should we include this in the v6 milestone?

@siriwatknp
Copy link
Member

@siriwatknp the logic on the filter is getting more complex, should we extract a utility for it so it can be shared throughout the components?

Also, should we include this in the v6 milestone?

Yes, please. I think it's a good addition but not a blocker for beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants