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

[Grid] Migrate to emotion #24395

Merged
merged 24 commits into from
Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from 23 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
3 changes: 2 additions & 1 deletion docs/pages/api-docs/grid.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
},
"default": "0"
},
"sx": { "type": { "name": "object" } },
"wrap": {
"type": {
"name": "enum",
Expand Down Expand Up @@ -143,5 +144,5 @@
"filename": "/packages/material-ui/src/Grid/Grid.js",
"inheritance": null,
"demos": "<ul><li><a href=\"/components/grid/\">Grid</a></li></ul>",
"styledComponent": false
"styledComponent": true
}
9 changes: 8 additions & 1 deletion docs/scripts/buildApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -709,8 +709,15 @@ async function updateStylesDefinition(context: {
if (members) {
styles.descriptions = styles.descriptions || {};
members.forEach((member) => {
const className = ((member as babel.types.TSPropertySignature)
let className = ((member as babel.types.TSPropertySignature)
.key as babel.types.Identifier).name;

if (!className) {
// Necessary for classes defined as kebab case
className = ((member as babel.types.TSPropertySignature)
.key as babel.types.StringLiteral).value;
}

styles.classes.push(className);
if (member.leadingComments) {
styles.descriptions[className] = trimComment(member.leadingComments[0].value);
Expand Down
1 change: 1 addition & 0 deletions docs/translations/api-docs/grid/grid.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"md": "Defines the number of grids the component is going to use. It&#39;s applied for the <code>md</code> breakpoint and wider screens if not overridden.",
"sm": "Defines the number of grids the component is going to use. It&#39;s applied for the <code>sm</code> breakpoint and wider screens if not overridden.",
"spacing": "Defines the space between the type <code>item</code> component. It can only be used on a type <code>container</code> component.",
"sx": "The system prop that allows defining system overrides as well as additional CSS styles. See the <a href=\"/system/basics/#the-sx-prop\">`sx` page</a> for more details.",
"wrap": "Defines the <code>flex-wrap</code> style property. It&#39;s applied for all screen sizes.",
"xl": "Defines the number of grids the component is going to use. It&#39;s applied for the <code>xl</code> breakpoint and wider screens.",
"xs": "Defines the number of grids the component is going to use. It&#39;s applied for all the screen sizes with the lowest priority.",
Expand Down
7 changes: 6 additions & 1 deletion packages/material-ui-utils/src/requirePropFactory.d.ts
Original file line number Diff line number Diff line change
@@ -1 +1,6 @@
export default function requirePropFactory(componentNameInError: string): any;
import * as React from 'react';

export default function requirePropFactory(
componentNameInError: string,
Component?: React.ComponentType
): any;
18 changes: 16 additions & 2 deletions packages/material-ui-utils/src/requirePropFactory.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export default function requirePropFactory(componentNameInError) {
export default function requirePropFactory(componentNameInError, Component) {
if (process.env.NODE_ENV === 'production') {
return () => null;
}
Expand All @@ -12,14 +12,28 @@ export default function requirePropFactory(componentNameInError) {
) => {
const propFullNameSafe = propFullName || propName;

// eslint-disable-next-line react/forbid-foreign-prop-types
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've got this failing test now https://app.circleci.com/pipelines/github/mui-org/material-ui/35659/workflows/8454c729-bc8f-49ba-8ae5-9fde972ca739/jobs/215603

Also, not sure how smart it is to disable this rule - https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-foreign-prop-types.md

I will revert the changes related to this and open a separte PR on this tomorrow.

Copy link
Member

@oliviertassinari oliviertassinari Jan 14, 2021

Choose a reason for hiding this comment

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

Also, not sure how smart it is to disable this rule - https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-foreign-prop-types.md

The eslint rule docs links https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types to the why. It's fine in our case. The block is wrapped with the process.env.NODE_ENV === 'production' condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, anyway will fix it separately to keep this PR clean

const defaultTypeChecker = Component?.propTypes?.[propFullNameSafe];
let defaultTypeCheckerResult = null;

if (defaultTypeChecker) {
defaultTypeCheckerResult = defaultTypeChecker(
props,
propName,
componentName,
location,
propFullName,
);
}

if (typeof props[propName] !== 'undefined' && !props[requiredProp]) {
return new Error(
`The prop \`${propFullNameSafe}\` of ` +
`\`${componentNameInError}\` can only be used together with the \`${requiredProp}\` prop.`,
);
}

return null;
return defaultTypeCheckerResult;
};
return requireProp;
}
27 changes: 27 additions & 0 deletions packages/material-ui-utils/src/requirePropFactory.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect } from 'chai';
import { spy } from 'sinon';
import requirePropFactory from './requirePropFactory';

describe('requirePropFactory', () => {
Expand Down Expand Up @@ -83,6 +84,32 @@ describe('requirePropFactory', () => {
});
});
});

it('should chain the proptypes with the default prop types coming from the component', () => {
const Test = () => null;
const mock = spy();
Test.propTypes = {
test: mock,
};

const customProps = {};
const customPropName = 'test';
customProps[customPropName] = true;

const customRequireProp = requirePropFactory('Test', Test);

const result = customRequireProp('otherProp');
result(customProps, customPropName, undefined, undefined, undefined);

expect(mock.callCount).to.equal(1);
expect(mock.args[0]).to.deep.equal([
customProps,
customPropName,
undefined,
undefined,
undefined,
]);
});
});
});
});
2 changes: 1 addition & 1 deletion packages/material-ui/src/Button/Button.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const overridesResolver = (props, styles) => {
};

const useUtilityClasses = (styleProps) => {
const { color, disableElevation, fullWidth, size, variant, classes = {} } = styleProps;
const { color, disableElevation, fullWidth, size, variant, classes } = styleProps;

const slots = {
root: [
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/ButtonBase/ButtonBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const overridesResolver = (props, styles) => {
};

const useUtilityClasses = (styleProps) => {
const { disabled, focusVisible, focusVisibleClassName, classes = {} } = styleProps;
const { disabled, focusVisible, focusVisibleClassName, classes } = styleProps;

const slots = {
root: ['root', disabled && 'disabled', focusVisible && 'focusVisible'],
Expand Down
6 changes: 6 additions & 0 deletions packages/material-ui/src/Grid/Grid.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import * as React from 'react';
import { SxProps } from '@material-ui/system';
import { Theme } from '../styles';
import { OverridableComponent, OverrideProps } from '../OverridableComponent';

export type GridItemsAlignment = 'flex-start' | 'center' | 'flex-end' | 'stretch' | 'baseline';
Expand Down Expand Up @@ -170,6 +172,10 @@ export interface GridTypeMap<P = {}, D extends React.ElementType = 'div'> {
* @default 0
*/
spacing?: GridSpacing;
/**
* The system prop that allows defining system overrides as well as additional CSS styles.
*/
sx?: SxProps<Theme>;
/**
* Defines the `flex-wrap` style property.
* It's applied for all screen sizes.
Expand Down
Loading