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

[docs] Remove wrong migration instruction #22710

Merged
merged 4 commits into from Sep 25, 2020

Conversation

oliviertassinari
Copy link
Member

A continuation of the discussion on bc45825#r42461020. From what I understand, our best option is to no support the previous case. Happy to consider alternatives.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 23, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 23, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 4259c81

@mbrookes
Copy link
Member

Should we also add a note to the customization/spacing page "a function" section that the function must return a string?

@oliviertassinari
Copy link
Member Author

Why does it need to return a string? If a number is returned it will assume px. Maybe it should mention the default unit instead?

@oliviertassinari
Copy link
Member Author

@mbrookes There is another case that is unclear, what's the expected behavior when you provide an array for the spacing? The issue with the typing is to get

https://github.com/mui-org/material-ui/blob/af9f26f333410c0f6e54437370ad384a3337be18/packages/material-ui/src/styles/createSpacing.d.ts#L6

be a number or a string, depending on the theme option input. I don't see how to make it happen.

@eps1lon
Copy link
Member

eps1lon commented Sep 24, 2020

be a number or a string, depending on the theme option input. I don't see how to make it happen.

Could you add a runtime test for that behavior? It's not clear to me how spacing would ever return a number.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 24, 2020

@eps1lon Spacing used to return a number or a string, it's no longer possible, it can only be a string now. We have been discussing with Matt how to bring this behavior back, we couldn't see a simple approach with the types.

@eps1lon
Copy link
Member

eps1lon commented Sep 24, 2020

We have been discussing with Matt how to bring this behavior back, we couldn't see a simple approach with the types.

Be aware that any code that's hard to express with types is also hard to explain to humans. What problem would overloading solve here?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 24, 2020

Be aware that any code that's hard to express with types is also hard to explain to humans.

@eps1lon Agree, this is why I'm leaning toward giving up on a way to have v4 behavior, going for all-in in the new string approach.

What problem would overloading solve here?

Being able to have:

const theme = createMuiTheme({
  spacing: factor => 8 * factor,
});

theme.spacing(2); // = 8

right now we have:

const theme = createMuiTheme({
  spacing: factor => 8 * factor,
});

theme.spacing(2); // = 8px

@eps1lon
Copy link
Member

eps1lon commented Sep 24, 2020

I understand the different return types. I'm asking what you'd do with this and why this problem should only be solved for a single argument.

@oliviertassinari
Copy link
Member Author

I understand the different return types. I'm asking what you'd do with this and why this problem should only be solved for a single argument.

Oh, sorry, you were on the second order thinking. I don't know, also a reason why my proposal: so we get feedback and compeling use cases from developers 😁.

Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
@eps1lon eps1lon merged commit 0acf4de into mui:next Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants