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] Array reject on spacing transformation fixed #19900

Merged
merged 3 commits into from
Mar 1, 2020
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
2 changes: 1 addition & 1 deletion docs/src/pages/customization/spacing/spacing.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ theme.spacing(2); // = 0.25 * 2rem = 0.5rem = 8px

```js
const theme = createMuiTheme({
spacing: factor => [0, 4, 8, 16, 32, 64][factor],
spacing: [0, 4, 8, 16, 32, 64],
});

theme.spacing(2); // = 8
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui-system/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export { default as shadows } from './shadows';
export { default as sizing } from './sizing';
export * from './sizing';
export { default as spacing } from './spacing';
export * from './spacing';
export { default as style } from './style';
export { default as typography } from './typography';
export * from './typography';
15 changes: 12 additions & 3 deletions packages/material-ui-system/src/spacing.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,20 @@ const spacingKeys = [
'paddingY',
];

function getTransformer(theme) {
export function createUnarySpacing(theme) {
const themeSpacing = theme.spacing || 8;

if (typeof themeSpacing === 'number') {
return abs => themeSpacing * abs;
return abs => {
if (process.env.NODE_ENV !== 'production') {
if (typeof abs !== 'number') {
console.error(
`@material-ui/system: expected spacing argument to be a number, got ${abs}.`,
);
}
}
return themeSpacing * abs;
};
}

if (Array.isArray(themeSpacing)) {
Expand Down Expand Up @@ -144,7 +153,7 @@ function getStyleFromPropValue(cssProperties, transformer) {

function spacing(props) {
const theme = props.theme;
const transformer = getTransformer(theme);
const transformer = createUnarySpacing(theme);

return Object.keys(props)
.map(prop => {
Expand Down
34 changes: 8 additions & 26 deletions packages/material-ui/src/styles/createSpacing.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { createUnarySpacing } from '@material-ui/system';

let warnOnce;

export default function createSpacing(spacingInput = 8) {
Expand All @@ -6,32 +8,12 @@ export default function createSpacing(spacingInput = 8) {
return spacingInput;
}

// All components align to an 8dp square baseline grid for mobile, tablet, and desktop.
// https://material.io/design/layout/understanding-layout.html#pixel-density
let transform;

if (typeof spacingInput === 'function') {
transform = spacingInput;
} else {
if (process.env.NODE_ENV !== 'production') {
if (typeof spacingInput !== 'number') {
console.error(
[
`Material-UI: the \`theme.spacing\` value (${spacingInput}) is invalid.`,
'It should be a number or a function.',
].join('\n'),
);
}
}
transform = factor => {
if (process.env.NODE_ENV !== 'production') {
if (typeof factor !== 'number') {
console.error(`Expected spacing argument to be a number, got ${factor}`);
}
}
return spacingInput * factor;
};
}
// Material Design layouts are visually balanced. Most measurements align to an 8dp grid applied, which aligns both spacing and the overall layout.
// Smaller components, such as icons and type, can align to a 4dp grid.
// https://material.io/design/layout/understanding-layout.html#usage
const transform = createUnarySpacing({
spacing: spacingInput,
});

const spacing = (...args) => {
if (process.env.NODE_ENV !== 'production') {
Expand Down
36 changes: 19 additions & 17 deletions packages/material-ui/src/styles/createSpacing.test.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
import { assert } from 'chai';
import { expect } from 'chai';
import createSpacing from './createSpacing';
import consoleErrorMock from 'test/utils/consoleErrorMock';

describe('createSpacing', () => {
it('should work as expected', () => {
let spacing;
spacing = createSpacing();
assert.strictEqual(spacing(1), 8);
expect(spacing(1)).to.equal(8);
spacing = createSpacing(10);
assert.strictEqual(spacing(1), 10);
spacing = createSpacing(factor => [0, 8, 16][factor]);
assert.strictEqual(spacing(2), 16);
expect(spacing(1)).to.equal(10);
spacing = createSpacing([0, 8, 16]);
expect(spacing(2)).to.equal(16);
spacing = createSpacing(factor => factor ** 2);
assert.strictEqual(spacing(2), 4);
expect(spacing(2)).to.equal(4);
spacing = createSpacing(factor => `${0.25 * factor}rem`);
assert.strictEqual(spacing(2), '0.5rem');
expect(spacing(2)).to.equal('0.5rem');
});

it('should support recursion', () => {
Expand All @@ -25,17 +25,17 @@ describe('createSpacing', () => {
it('should support a default value when no arguments are provided', () => {
let spacing;
spacing = createSpacing();
assert.strictEqual(spacing(), 8);
expect(spacing()).to.equal(8);
spacing = createSpacing(factor => `${0.25 * factor}rem`);
assert.strictEqual(spacing(), '0.25rem');
expect(spacing()).to.equal('0.25rem');
});

it('should support multiple arguments', () => {
let spacing;
spacing = createSpacing();
assert.strictEqual(spacing(1, 2), '8px 16px');
expect(spacing(1, 2)).to.equal('8px 16px');
spacing = createSpacing(factor => `${0.25 * factor}rem`);
assert.strictEqual(spacing(1, 2), '0.25rem 0.5rem');
expect(spacing(1, 2)).to.equal('0.25rem 0.5rem');
});

describe('warnings', () => {
Expand All @@ -47,20 +47,22 @@ describe('createSpacing', () => {
consoleErrorMock.reset();
});

// TODO v5: remove
it('should warn for the deprecated API', () => {
const spacing = createSpacing(11);
assert.strictEqual(spacing.unit, 11);
assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(consoleErrorMock.args()[0][0], 'theme.spacing.unit usage has been deprecated');
expect(spacing.unit).to.equal(11);
expect(consoleErrorMock.callCount()).to.equal(1);
expect(consoleErrorMock.args()[0][0]).to.include(
'theme.spacing.unit usage has been deprecated',
);
});

it('should warn for wrong input', () => {
createSpacing({
unit: 4,
});
assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(
consoleErrorMock.args()[0][0],
expect(consoleErrorMock.callCount()).to.equal(1);
expect(consoleErrorMock.args()[0][0]).to.include(
'the `theme.spacing` value ([object Object]) is invalid',
);
});
Expand Down