Skip to content

Commit

Permalink
[system] Array reject on spacing transformation fixed (#19900)
Browse files Browse the repository at this point in the history
  • Loading branch information
Weslen do Nascimento authored Mar 1, 2020
1 parent 41a08d2 commit 0033bb3
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 47 deletions.
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

0 comments on commit 0033bb3

Please sign in to comment.