Fix hardcoded TextField size prop

[PedroPerpetua]

This fixes the issue the issue with the TextField size prop not being override-able by theme. RA's default theme actually already sets the MuiTextField size prop default value as "small", so it's redundant to have it here.

However, in a possibly unrelated note, it may be benefitial to extend RA's documentation and themeing options with a re-export of MUI's createTheme function, that allows the deep-merging of themes, or maybe recommend lodash.merge to set up themes.

With this PR, overriding the theme's TextField variant as per RA's docs will result in the size="small" property to be lost.

Closes #9546

[djhi]

Thanks!

[adguernier]

I agree too since RA's defaultTheme sets the MuiTextField defaultProps to "small

[fzaninotto]

This is the right code, thanks! However we need to document this change in the upgrade guide. People with a custom theme will see their inputs size change from small to medium, and this is a regression. We also need to document the custom size somewhere in the Inputs documentation.

We usually put the doc and the code in the same PR, would you mind doing these changes in your PR?

[PedroPerpetua]

We usually put the doc and the code in the same PR, would you mind doing these changes in your PR?

Yeah, I can give it a spin later today!

[PedroPerpetua]

Okay, I've updated the documentation, and took the liberty of changing the theme configurations to use deepmerge and createTheme from MUI instead of object deconstruction, as suggested by MUI's docs. I find this to be a much better way for users to set up their own themes when basing themselves on the default themes, and this easily solves the issue of overriding a couple deep properties of the theme.

For example, using deepmerge, you can easily override just the variant prop in a TextField while keeping the size prop the same.

It's a "big breaking change" (old way would still work). I hope this is fine.

[fzaninotto]

You're still missing a section in the v5 upgrade guide for people with a custom theme in v4.

[fzaninotto]

you shouldn't call createTheme here as our theme provider does it, too.

[fzaninotto]

We don't need the deepMerge here, as we know what we're merging. I prefer the old version.

[PedroPerpetua]

deepMerge here allows new changes to be made more easily in the future, in my opinion. Imagine, as a crude example, you want the darkTheme to have their textfields with a width of 50% by default.

To do this change without using deepmerge:

const defaultDarkTheme: RaThemeOptions = {
    ...defaultThemeInvariants,
    palette: {
        mode: 'dark',
        primary: {
            main: '#90caf9',
        },
        background: {
            default: '#313131',
        },
    },
+    components: {
+        ...defaultThemeInvariants.components,
+        MuiTextField: {
+            ...defaultThemeInvariants.components.MuiTextField,
+            defaultProps: {
+                ...defaultThemeInvariants.components.MuiTextField.defaultProps,
+                sx: {
                    /* 
                    If you later add a default sx to defaultThemeInvariants,
                    you need to remember to come here and add the following line:

+                    ...defaultThemeInvariants.components.MuiTextField.defaultProps.sx,
                    
                    */
+                    width: "50%"
+                }
+            },
+        },
+    }
}

With deepmerge:

export const defaultDarkTheme: RaThemeOptions = deepmerge(defaultThemeInvariants, {
        palette: {
            mode: 'dark',
            primary: {
                main: '#90caf9',
            },
            background: {
                default: '#313131',
            },
        },
+        components: {
+            MuiTextField: {
+                defaultProps: {
+                    sx: {
+                        width: "50%"
+                    }
+                }
+            }
+        }
    })
);

The retro-active update mentioned in the comment being specially worrying in my opinion.

[PedroPerpetua]

Alright, I'll give the review a spin, hopefully monday.

[PedroPerpetua]

Okay, I've added the useTheme changes to the migration guide; I also went ahead and removed the legacy code on useTheme and the toggleThemeButton fully; it's a bit out of the original scope of this PR, but given that the original issue was a 1 line bugfix, I figured they can go along and the PR can be renamed as you find appropriate.

Regarding this:

We don't need the deepMerge here, as we know what we're merging. I prefer the old version.

@fzaninotto

I left a response to your comment; waiting on a reply to know if I should revert the changes there or keep them.

[PedroPerpetua]

BUMPing this since I'm still awaiting on a reply from @fzaninotto to know if it needs more changes or is ready 😉

[fzaninotto]

We're getting close to the merge!

[PedroPerpetua]

Fixed the latest comments and the merge conflits

[fzaninotto]

Thanks a lot!