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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/mui-joy/src/Typography/Typography.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ describe('<Typography />', () => {
});
});

it('combines system properties with the sx prop', () => {
it('combines system properties with the sx prop', function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}
Comment on lines +102 to +104
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

const { container } = render(<Typography mt={2} mr={1} sx={{ marginRight: 5, mb: 2 }} />);

expect(container.firstChild).toHaveComputedStyle({
Expand Down
2 changes: 1 addition & 1 deletion packages/mui-joy/src/styles/CssVarsProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ describe('[Joy] CssVarsProvider', () => {
</CssVarsProvider>,
);

expect(container.firstChild?.textContent).to.equal('16px');
expect(container.firstChild?.textContent).to.equal('calc(2 * var(--joy-spacing))');
});
});

Expand Down
1 change: 1 addition & 0 deletions packages/mui-joy/src/styles/defaultTheme.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('defaultTheme', () => {
'shouldSkipGeneratingVar',
'generateStyleSheets',
'generateThemeVars',
'generateSpacing',
'applyStyles',
]).to.includes(field);
});
Expand Down
46 changes: 46 additions & 0 deletions packages/mui-joy/src/styles/extendTheme.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('extendTheme', () => {
'shouldSkipGeneratingVar',
'generateStyleSheets',
'generateThemeVars',
'generateSpacing',
'applyStyles',
]).to.includes(field);
});
Expand Down Expand Up @@ -106,6 +107,51 @@ describe('extendTheme', () => {
});
});

describe('spacing', () => {
it('produce spacing token by default', () => {
const theme = extendTheme();
expect(theme.vars.spacing).to.equal('var(--joy-spacing, 8px)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--joy-spacing, 8px))');
});

it('turn number to pixel', () => {
const theme = extendTheme({ spacing: 4 });
expect(theme.vars.spacing).to.equal('var(--joy-spacing, 4px)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--joy-spacing, 4px))');
});

it('can be customized as a string', () => {
const theme = extendTheme({ spacing: '0.5rem' });
expect(theme.vars.spacing).to.equal('var(--joy-spacing, 0.5rem)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--joy-spacing, 0.5rem))');
});

it('uses the provided value if it is a string', () => {
const theme = extendTheme({ spacing: '0.5rem' });
expect(theme.spacing('1rem')).to.equal('1rem');
});

it('can be customized as an array', () => {
const theme = extendTheme({ spacing: [0, 1, 2, 4, 8, 16, 32] });
expect(theme.vars.spacing).to.deep.equal([
'var(--joy-spacing-0, 0px)',
'var(--joy-spacing-1, 1px)',
'var(--joy-spacing-2, 2px)',
'var(--joy-spacing-3, 4px)',
'var(--joy-spacing-4, 8px)',
'var(--joy-spacing-5, 16px)',
'var(--joy-spacing-6, 32px)',
]);
expect(theme.spacing(2)).to.equal('var(--joy-spacing-2, 2px)');
});

it('can be customized as a function', () => {
const theme = extendTheme({ spacing: (factor) => `${0.25 * factor}rem` });
expect(theme.vars.spacing).to.deep.equal('var(--joy-spacing, 0.25rem)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--joy-spacing, 0.25rem))');
});
});

describe('theme.unstable_sx', () => {
const { render } = createRenderer();

Expand Down
26 changes: 23 additions & 3 deletions packages/mui-joy/src/styles/extendTheme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
SxConfig,
} from '@mui/system';
import { unstable_applyStyles as applyStyles } from '@mui/system/createTheme';
import { createUnarySpacing } from '@mui/system/spacing';
import defaultSxConfig from './sxConfig';
import colors from '../colors';
import defaultShouldSkipGeneratingVar from './shouldSkipGeneratingVar';
Expand Down Expand Up @@ -43,6 +44,22 @@ type Partial3Level<T> = {
};
};

function getSpacingVal(spacingInput: SpacingOptions | string | undefined) {
if (typeof spacingInput === 'number') {
return `${spacingInput}px`;
}
if (typeof spacingInput === 'string') {
return spacingInput;
}
if (typeof spacingInput === 'function') {
return getSpacingVal(spacingInput(1));
}
if (Array.isArray(spacingInput)) {
return spacingInput;
}
return '8px';
}

export type ColorSystemOptions = Partial3Level<
MergeDefault<ColorSystem, { palette: PaletteOptions }>
> & {
Expand Down Expand Up @@ -586,7 +603,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
),
cssVarPrefix,
getCssVar,
spacing: createSpacing(spacing),
spacing: getSpacingVal(spacing),
} as unknown as Theme & { attribute: string; colorSchemeSelector: string }; // Need type casting due to module augmentation inside the repo

/**
Expand Down Expand Up @@ -646,15 +663,18 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
};

const { vars, generateThemeVars, generateStyleSheets } = prepareCssVars<Theme, ThemeVars>(
// @ts-ignore property truDark is missing from colorSchemes
{ colorSchemes, ...mergedScales },
theme,
parserConfig,
);
theme.attribute = 'data-joy-color-scheme';
theme.colorSchemeSelector = ':root';
theme.vars = vars;
theme.generateThemeVars = generateThemeVars;
theme.generateStyleSheets = generateStyleSheets;
theme.generateSpacing = function generateSpacing() {
return createSpacing(spacing, createUnarySpacing(this));
};
theme.spacing = theme.generateSpacing();
theme.unstable_sxConfig = {
...defaultSxConfig,
...themeOptions?.unstable_sxConfig,
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-joy/src/styles/styleUtils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ describe('getThemeValue', () => {
it('return correct value if shorthand is provided', () => {
expect(
resolveSxValue({ theme: defaultTheme, ownerState: { sx: { p: 2 } } }, ['p']),
).to.deep.equal({ p: '16px' }); // default theme.spacing(2) = '16px'
).to.deep.equal({ p: 'calc(2 * var(--joy-spacing, 8px))' });
});

it('return correct value if number is provided', () => {
expect(
resolveSxValue({ theme: defaultTheme, ownerState: { sx: { padding: 2 } } }, ['padding']),
).to.deep.equal({ padding: '16px' });
).to.deep.equal({ padding: 'calc(2 * var(--joy-spacing, 8px))' });
});

it('return correct value if css value is provided', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/mui-joy/src/styles/types/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export interface Theme extends ThemeScales, RuntimeColorSystem {
getColorSchemeSelector: (colorScheme: DefaultColorScheme | ExtendedColorScheme) => string;
generateThemeVars: () => ThemeVars;
generateStyleSheets: () => Record<string, any>[];
generateSpacing: () => Spacing;
/**
* A function to determine if the key, value should be attached as CSS Variable
* `keys` is an array that represents the object path keys.
Expand Down
3 changes: 3 additions & 0 deletions packages/mui-material/src/Grid/Grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if (Number.isNaN(parse)) {
return val;
}
return `${parse}${String(val).replace(String(parse), '') || 'px'}`;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/mui-material/src/styles/CssVarsProvider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ describe('[Material UI] CssVarsProvider', () => {
</CssVarsProvider>,
);

expect(container.firstChild?.textContent).to.equal('16px');
expect(container.firstChild?.textContent).to.equal('calc(2 * var(--mui-spacing))');
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ export interface ThemeVars {
overlays: Overlays;
shadows: Shadows;
shape: Theme['shape'];
spacing: string;
zIndex: ZIndex;
}

Expand Down Expand Up @@ -431,6 +432,7 @@ export interface CssVarsTheme extends ColorSystem {
getColorSchemeSelector: (colorScheme: SupportedColorScheme) => string;
generateThemeVars: () => ThemeVars;
generateStyleSheets: () => Array<Record<string, any>>;
generateSpacing: () => Theme['spacing'];

// Default theme tokens
spacing: Theme['spacing'];
Expand Down
28 changes: 24 additions & 4 deletions packages/mui-material/src/styles/experimental_extendTheme.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import deepmerge from '@mui/utils/deepmerge';
import {
unstable_createGetCssVar as systemCreateGetCssVar,
unstable_prepareCssVars as prepareCssVars,
} from '@mui/system';
import { unstable_createGetCssVar as systemCreateGetCssVar, createSpacing } from '@mui/system';
import { createUnarySpacing } from '@mui/system/spacing';
import { prepareCssVars } from '@mui/system/cssVars';
import styleFunctionSx, {
unstable_defaultSxConfig as defaultSxConfig,
} from '@mui/system/styleFunctionSx';
Expand Down Expand Up @@ -63,6 +62,22 @@ function setColorChannel(obj, key) {
}
}

function getSpacingVal(spacingInput) {
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
if (typeof spacingInput === 'number') {
return `${spacingInput}px`;
}
if (typeof spacingInput === 'string') {
return spacingInput;
}
if (typeof spacingInput === 'function') {
return getSpacingVal(spacingInput(1));
}
if (Array.isArray(spacingInput)) {
return spacingInput;
}
return '8px';
}

const silent = (fn) => {
try {
return fn();
Expand Down Expand Up @@ -124,6 +139,7 @@ export default function extendTheme(options = {}, ...args) {
overlays: colorSchemesInput.dark?.overlays || defaultDarkOverlays,
},
},
spacing: getSpacingVal(input.spacing),
};

Object.keys(theme.colorSchemes).forEach((key) => {
Expand Down Expand Up @@ -414,7 +430,11 @@ export default function extendTheme(options = {}, ...args) {
});
theme.generateThemeVars = generateThemeVars;
theme.generateStyleSheets = generateStyleSheets;
theme.generateSpacing = function generateSpacing() {
return createSpacing(input.spacing, createUnarySpacing(this));
};
Comment on lines +433 to +435
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));

theme.shouldSkipGeneratingVar = shouldSkipGeneratingVar;
theme.unstable_sxConfig = {
...defaultSxConfig,
Expand Down
45 changes: 45 additions & 0 deletions packages/mui-material/src/styles/experimental_extendTheme.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,51 @@ describe('experimental_extendTheme', () => {
});
});

describe('spacing', () => {
it('produce spacing token by default', () => {
const theme = extendTheme();
expect(theme.vars.spacing).to.equal('var(--mui-spacing, 8px)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--mui-spacing, 8px))');
});

it('turn number to pixel', () => {
const theme = extendTheme({ spacing: 4 });
expect(theme.vars.spacing).to.equal('var(--mui-spacing, 4px)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--mui-spacing, 4px))');
});

it('can be customized as a string', () => {
const theme = extendTheme({ spacing: '0.5rem' });
expect(theme.vars.spacing).to.equal('var(--mui-spacing, 0.5rem)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--mui-spacing, 0.5rem))');
});

it('uses the provided value if it is a string', () => {
const theme = extendTheme({ spacing: '0.5rem' });
expect(theme.spacing('1rem')).to.equal('1rem');
});

it('can be customized as an array', () => {
const theme = extendTheme({ spacing: [0, 1, 2, 4, 8, 16, 32] });
expect(theme.vars.spacing).to.deep.equal([
'var(--mui-spacing-0, 0px)',
'var(--mui-spacing-1, 1px)',
'var(--mui-spacing-2, 2px)',
'var(--mui-spacing-3, 4px)',
'var(--mui-spacing-4, 8px)',
'var(--mui-spacing-5, 16px)',
'var(--mui-spacing-6, 32px)',
]);
expect(theme.spacing(2)).to.equal('var(--mui-spacing-2, 2px)');
});

it('can be customized as a function', () => {
const theme = extendTheme({ spacing: (factor) => `${0.25 * factor}rem` });
expect(theme.vars.spacing).to.deep.equal('var(--mui-spacing, 0.25rem)');
expect(theme.spacing(2)).to.equal('calc(2 * var(--mui-spacing, 0.25rem))');
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
});
});

it('shallow merges multiple arguments', () => {
const theme = extendTheme({ foo: 'I am foo' }, { bar: 'I am bar' });
expect(theme.foo).to.equal('I am foo');
Expand Down
2 changes: 2 additions & 0 deletions packages/mui-system/src/createTheme/createSpacing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ describe('createSpacing', () => {
expect(spacing(2)).to.equal('4px');
spacing = createSpacing((factor: number) => `${0.25 * factor}rem`);
expect(spacing(2)).to.equal('0.5rem');
spacing = createSpacing('0.5rem');
expect(spacing(2)).to.equal('calc(2 * 0.5rem)');
});

it('should support recursion', () => {
Expand Down
18 changes: 10 additions & 8 deletions packages/mui-system/src/createTheme/createSpacing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { createUnarySpacing } from '../spacing';

export type SpacingOptions =
| number
| string
| Spacing
| ((abs: number) => number | string)
| ((abs: number | string) => number | string)
Expand All @@ -24,18 +25,19 @@ export interface Spacing {
): string;
}

export default function createSpacing(spacingInput: SpacingOptions = 8): Spacing {
// Already transformed.
if ((spacingInput as any).mui) {
return spacingInput as Spacing;
}

export default function createSpacing(
spacingInput: SpacingOptions = 8,
// Material Design layouts are visually balanced. Most measurements align to an 8dp grid, which aligns both spacing and the overall layout.
// Smaller components, such as icons, can align to a 4dp grid.
// https://m2.material.io/design/layout/understanding-layout.html
const transform = createUnarySpacing({
transform = createUnarySpacing({
spacing: spacingInput,
});
}),
): Spacing {
// Already transformed.
if ((spacingInput as any).mui) {
return spacingInput as Spacing;
}

const spacing = (...argsInput: ReadonlyArray<number | string>): string => {
if (process.env.NODE_ENV !== 'production') {
Expand Down
3 changes: 3 additions & 0 deletions packages/mui-system/src/cssVars/createCssVarsProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ export default function createCssVarsProvider(options) {
cssVarPrefix,
vars: themeVars,
};
if (typeof theme.generateSpacing === 'function') {
theme.spacing = theme.generateSpacing();
}

// 4. Resolve the color scheme and merge it to the theme
Object.entries(colorSchemes).forEach(([key, scheme]) => {
Expand Down
15 changes: 15 additions & 0 deletions packages/mui-system/src/cssVars/cssVarsParser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,21 @@ describe('cssVarsParser', () => {
});
});

it('varsWithDefaults are suffixed with px from array', () => {
const { varsWithDefaults } = cssVarsParser({
spacing: [0, 1, 2, 6, 16],
});
expect(varsWithDefaults).to.deep.equal({
spacing: [
'var(--spacing-0, 0px)',
'var(--spacing-1, 1px)',
'var(--spacing-2, 2px)',
'var(--spacing-3, 6px)',
'var(--spacing-4, 16px)',
],
});
});

it('should add a fallback value', () => {
const { varsWithDefaults } = cssVarsParser({
palette: {
Expand Down
Loading
Loading