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

[system] Add spacing theme token to be used in theme.spacing() #40224

Merged
merged 17 commits into from
Mar 27, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Dec 18, 2023

part of #40225

closes #37911

// before
theme.spacing(1) // 8px

// after
theme.spacing(1) // calc(1 * var(--mui-spacing))

Changes

When using extendTheme({ spacing: string | number }), a new token (--mui-spacing) is created for use with CSS calc function.

Screen.Recording.2567-03-13.at.15.52.46.mov

@siriwatknp siriwatknp added the customization: theme Centered around the theming features label Dec 18, 2023
@mui-bot
Copy link

mui-bot commented Dec 18, 2023

Netlify deploy preview

https://deploy-preview-40224--material-ui.netlify.app/

@material-ui/core: parsed: +0.08% , gzip: +0.08%
@mui/joy: parsed: +0.07% , gzip: +0.11%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 077fd51

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 20, 2023
@siriwatknp siriwatknp changed the title [WIP] Add spacingSize theme token to be used in theme.spacing() [WIP] Add spacing theme token to be used in theme.spacing() Dec 20, 2023
@danilo-leal danilo-leal changed the title [WIP] Add spacing theme token to be used in theme.spacing() [WIP][system] Add spacing theme token to be used in theme.spacing() Dec 21, 2023
@danilo-leal danilo-leal added the package: system Specific to @mui/system label Dec 21, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 13, 2024
@siriwatknp siriwatknp added the new feature New feature or request label Mar 13, 2024
@siriwatknp siriwatknp changed the title [WIP][system] Add spacing theme token to be used in theme.spacing() [system] Add spacing theme token to be used in theme.spacing() Mar 13, 2024
@siriwatknp siriwatknp marked this pull request as ready for review March 13, 2024 09:53
@siriwatknp siriwatknp requested a review from DiegoAndai March 13, 2024 09:53
@siriwatknp
Copy link
Member Author

@DiegoAndai This is not a breaking change but do you think it should a part of v5 or v6?

@siriwatknp
Copy link
Member Author

Moved to v6.

@siriwatknp siriwatknp closed this Mar 13, 2024
@DiegoAndai
Copy link
Member

@DiegoAndai This is not a breaking change but do you think it should a part of v5 or v6?

Being so close to v5 freezing, I agree with you to not merge it to master and wait for v6.

@siriwatknp siriwatknp reopened this Mar 20, 2024
@@ -27,6 +27,9 @@ import gridClasses, { getGridUtilityClass } from './gridClasses';

function getOffset(val) {
const parse = parseFloat(val);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val could be a CSS expression.

@siriwatknp siriwatknp changed the base branch from master to next March 22, 2024 07:21
Comment on lines +102 to +104
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing margin values should use Karma

@DiegoAndai
Copy link
Member

@siriwatknp let me know when this is ready for review 😊

@siriwatknp
Copy link
Member Author

@siriwatknp let me know when this is ready for review 😊

It's ready!

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition! A couple of questions

if (typeof abs === 'string') {
return abs;
if (typeof themeSpacing === 'number' || typeof themeSpacing === 'string') {
return (val) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the variable name to reflect what we expect because the val is no longer an absolute of the user input. It's the user input without any modification.

@siriwatknp siriwatknp requested a review from DiegoAndai March 27, 2024 02:34
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@siriwatknp siriwatknp merged commit 04c8786 into mui:next Mar 27, 2024
19 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 29, 2024
tejasparkar pushed a commit to tejasparkar/material-ui that referenced this pull request Mar 30, 2024
Comment on lines +433 to +435
theme.generateSpacing = function generateSpacing() {
return createSpacing(input.spacing, createUnarySpacing(this));
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do this?

Suggested change
theme.generateSpacing = function generateSpacing() {
return createSpacing(input.spacing, createUnarySpacing(this));
};

Why do we need generateSpacing in the first place, and even more in the public API? It seems that using theme.spacing directly is simple enough.

theme.getColorSchemeSelector = (colorScheme) => `[${theme.attribute}="${colorScheme}"] &`;
theme.spacing = theme.generateSpacing();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
theme.spacing = theme.generateSpacing();
theme.spacing = createSpacing(input.spacing, createUnarySpacing(theme));

@oliviertassinari
Copy link
Member

There is a regression visible in the video description of these changes:

Screenshot 2024-06-08 at 20 28 28

I opened to try to fix it #42574.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features new feature New feature or request package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] Expose spacing unit as a CSS variable
5 participants