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

[theme] Fix spacing string arguments #23224

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

GuilleDF
Copy link
Contributor

@GuilleDF GuilleDF commented Oct 23, 2020

Bug breaking change behavior

  • [theme] Custom spacing functions need to handle all the arguments, e.g. auto was incorrectly omitted. This restores the previous behavior.

This resolves #21278. The change gives more control to the developers that choose to provide a function to the unit.

When passing a function to createSpacing, only number arguments were passed to the function, while strings were not.

Before:

const spacing = createSpacing((factor) => `${0.25 * factor}rem`);
expect(spacing(1, 'auto', 2, 3)).to.equal('0.25rem auto 0.5rem 0.75rem');

After:
With this PR we pass the strings to the function and expect the function to handle them:

spacing = createSpacing((factor) =>
  typeof factor === 'string' ? factor : `${0.25 * factor}rem`,
);
expect(spacing(1, 'auto', 2, 3)).to.equal('0.25rem auto 0.5rem 0.75rem');

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 23, 2020

Details of bundle changes

Generated by 🚫 dangerJS against cbc257c

@GuilleDF GuilleDF changed the title Fix spacing string arguments [Theming] Fix spacing string arguments Oct 23, 2020
@oliviertassinari oliviertassinari changed the title [Theming] Fix spacing string arguments [theme] Fix spacing string arguments Oct 24, 2020
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work breaking change and removed breaking change labels Oct 24, 2020
@oliviertassinari oliviertassinari merged commit 42183a9 into mui:next Oct 26, 2020
@oliviertassinari
Copy link
Member

@GuilleDF Thanks!

@GuilleDF GuilleDF deleted the fix-theme-spacing-string-args branch October 27, 2020 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Theming] theme.spacing doesn't accept multiple string arguments
3 participants