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

[pickers] DatePicker theme augmentation styleOverrides types not working #9969

Closed
2 tasks done
thany opened this issue Aug 8, 2023 · 9 comments · Fixed by #9978
Closed
2 tasks done

[pickers] DatePicker theme augmentation styleOverrides types not working #9969

thany opened this issue Aug 8, 2023 · 9 comments · Fixed by #9978
Assignees
Labels
component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature typescript

Comments

@thany
Copy link

thany commented Aug 8, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/mui-x-issue-template-forked-9l2w87?file=/src/App.tsx

Steps:

  1. Open the example above
  2. Hover the word root
  3. Try adding another one next to root using intellisense
  4. Try adding something inside root

Current behavior 😯

The object root in the above example appears to be unknown, but is allowed typings-wise. It's unknown, and therefor hovering it reveals typescript thinks it's just an ordinary empty object. Pressing Ctrl+Space on root reveals why - it's not in the typings:
image

And neither is any other child member of styleOverrides. This makes it near-impossible to know what I can do there. It's essentially as "blind" as plain Javascript.

Furthermore, hovering the styleOverrides object reveals its type:
image

So it says Partial<OverridesStyleRules<never, "MuiDatePicker", Omit<Theme, "components">>> - I'm pretty sure that never isn't supposed to be in there, as it also isn't there for regular MUI components, like I dunno, MuiInput.

And as a result, I can do pretty much anything in styleOverrides and none of it will show any errors, i.e. no type checking is being done, because probably the types aren't there. I can add a foo next to root, or a backgroundColor: 4 and it's all "good", because it doesn't check any of it. When I do the same for an MuiInput theme, it's all type-checked perfectly, and none of my shenanigans is accepted.

Expected behavior 🤔

That never probably shouldn't be there, and intellisense should work. Type checking should also work, it shouldn't allow unknown or incorrect things in styleOverrides.

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo
  System:
    OS: Windows 10 10.0.19045
  Binaries:
    Node: 18.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: Not Found
    npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 115.0.5790.171
    Edge: Spartan (44.19041.1266.0), Chromium (115.0.1901.200)
  npmPackages:
    @emotion/react:  11.11.1
    @emotion/styled:  11.11.0
    @mui/base:  5.0.0-alpha.124
    @mui/core-downloads-tracker:  5.11.16
    @mui/material:  5.11.16
    @mui/private-theming:  5.12.0
    @mui/styled-engine:  5.11.16
    @mui/styles:  5.12.0
    @mui/system:  5.11.16
    @mui/types:  7.2.4
    @mui/utils:  5.14.3
    @mui/x-date-pickers:  6.11.0
    @types/react:  18.2.17
    react:  18.2.0
    react-dom:  18.2.0
    typescript:  4.9.5

Order ID or Support key 💳 (optional)

No response

@thany thany added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 8, 2023
@zannager zannager added the component: pickers This is the name of the generic UI component, not the React module! label Aug 8, 2023
@alexfauquette
Copy link
Member

Those never are effectively in the codebase
https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/themeAugmentation/overrides.d.ts/#L67-L84

I'm not sure MuiDatePicker should support styleOverrides. As you can see, only defaultProps are tested in the spec file.
I guess, it's because the pickers are a group of smaller component, that you can customize using the styleOverrides of those smaller ones. For example MuiTimePickerToolbar

https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/themeAugmentation/themeAugmentation.spec.ts/#L539-L552

@LukasTy
Copy link
Member

LukasTy commented Aug 8, 2023

Thank you for opening this issue. 🙏
I'm not sure if we have this tracked or planned, but it is indeed a misleading API that we are exposing just for the sake of consistency.
All of the components that are within the themeAugmentation expose both defaultProps as well as styleOverrides, although, only the components that actually use any styled element do end up consuming those styleOverrides.
Our current solution for that is to define cases where a certain component does not consume the styleOverrides with the never type.
The existing solution is indeed misleading because it would just be more clear to remove the styleOverrides in our themeAugmentation and keep only defaultProps for such components.

This is somewhat of a breaking change, hence, we did not rush with a solution for it on our end.

@LukasTy
Copy link
Member

LukasTy commented Aug 8, 2023

@alexfauquette would you agree that not having styleOverrides as an option in TS for such cases would be the most correct approach?

@LukasTy LukasTy added typescript enhancement This is not a bug, nor a new feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 8, 2023
@LukasTy LukasTy changed the title DatePicker: theme augmentation types not working [pickers] DatePicker theme augmentation types not working Aug 8, 2023
@LukasTy LukasTy changed the title [pickers] DatePicker theme augmentation types not working [pickers] DatePicker theme augmentation styleOverrides types not working Aug 8, 2023
@alexfauquette
Copy link
Member

Yes but I don't have a strong opinion on styleOverrides topics since I rarely use them

@thany
Copy link
Author

thany commented Aug 9, 2023

A better solution is to have styleOverrides and then also have the component(s) consume it. Because currently it seems only possible to override styles, in this in the DatePicker, to either make it styled, or use numerous sx props. Neither of which are easy to reuse throughout an app, and neither are really a good way to apply themeing, iyam.

@LukasTy
Copy link
Member

LukasTy commented Aug 9, 2023

A better solution is to have styleOverrides and then also have the component(s) consume it.

Thank you for your suggestion.
Your idea has its merit and is worth exploring, especially given the fact that DataGrid is behaving like that, but it's quite a bit more complicated given how different picker components are re-using certain components.

We are aware of the problem that users are facing in deciding the best way to tackle styling overrides in certain situations.
The plan is to provide an interactive demo allowing users to choose the best way to override styles for a given (sub)component. 😉

In the meantime, could you clarify, which part of the pickers are you having problems styling?
Pickers' styling approach is modular. Let's say you want to override certain styles on the PickersDay component, you can easily do it by:

MuiPickersDay: {
  styleOverrides: {
    today: {
      backgroundColor: 'yellow',
    }
  }
}

This would apply the styling to all the (Date and Date Time) pickers variants in the application.

@thany
Copy link
Author

thany commented Aug 10, 2023

I was trying to style the input, so like the border color & width, and font color.

The interactive demo sounds like a cool idea btw 👍🏻

@LukasTy
Copy link
Member

LukasTy commented Aug 10, 2023

I was trying to style the input, so like the border color & width, and font color.

That's a use case that we haven't though of. As far as I can see, we do not have any unique classes or slots that would allow us to target specific pickers' text field. 🤔
Do you have a use case, where you want the picker's inputs to be styled differently from all the other inputs? 🤔
If yes, then targeting MuiDateField would allow changing the styles of the DatePicker input specifically.

@thany
Copy link
Author

thany commented Aug 11, 2023

The MuiDateField seems like a viable thing to target. But it does still feel like a workaround. I still feel like the MuiDatePicker should be able to provide styleOverrides for the classes its components are using.

Otoh, perhaps it's not such a bad idea to target generic subcomponents instead, just in case we need an additional component that also uses it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants