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 Container component and createContainer factory #32263

Merged
merged 35 commits into from May 11, 2022

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Apr 12, 2022

Adds a Container component in the system and re-exports from both Material UI and Joy UI. In addition it fixes a previous problem with the prop types generation (now prop types can be generated for the system components too cc @siriwatknp)

https://deploy-preview-32263--material-ui.netlify.app/system/react-container/

@mui-bot
Copy link

mui-bot commented Apr 12, 2022

Details of bundle changes

@material-ui/core: parsed: +0.10% , gzip: +0.09%
@material-ui/system: parsed: +4.62% , gzip: +4.17%
@mui/joy: parsed: +1.70% , gzip: +1.91%

Generated by 🚫 dangerJS against 5fffd9c

@danilo-leal danilo-leal added the package: system Specific to @mui/system label Apr 12, 2022
@mnajdova mnajdova added the on hold There is a blocker, we need to wait label Apr 14, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 15, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 21, 2022
@@ -13,6 +13,8 @@ describe('@mui/joy', () => {
});

it('should not have undefined exports', () => {
Object.keys(joy).forEach((exportKey) => expect(Boolean(joy[exportKey])).to.equal(true));
Object.keys(joy).forEach((exportKey) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to improve the error message to something more meaningful

@mnajdova mnajdova marked this pull request as ready for review April 21, 2022 12:50
@mnajdova mnajdova added component: Container The React component on hold There is a blocker, we need to wait and removed on hold There is a blocker, we need to wait labels Apr 22, 2022
@mnajdova
Copy link
Member Author

To be merged after #32456

@siriwatknp
Copy link
Member

siriwatknp commented Apr 26, 2022

I have 3 points from my side:

Folder structure:

I think the create... API should stay inside its folder. Ex:

/container/
  - createContainer.tsx (should we go with .tsx as first choice?)
  - container.tsx

Otherwise, when we have other components like Grid, Stack, ...etc. Every create... will be at root src folder.

And in Joy, Material UI we can import it like:

import { createContainer } from '@mui/system/Container';

same as createBox but we can refactor them later.

createContainer API

I think the responsibility of the system component creator is to provide props + classNames + styles to a styled component function.

The API options should be like this:

export default function createContainer(options = {}) {
  const {
    createStyledComponent,
    useThemeProps,
  } = options;
  
  const ContainerRoot = createStyledComponent(
    ({ theme, ownerState }) => ({ ... })
  );

  const Container = React.forwardRef(function Container(inProps, ref) {
    const props = useThemeProps(inProps);

    ...
    
    return (
      <ContainerRoot {...} />
    )
  });

With this, the consumers can control the styled component creation with the params that suit their needs.

// Material UI
const Container = createContainer({
  createStyledComponent: styled('div', {
    name: 'MuiContainer',
    slot: 'Root',
    overridesResolver: (props, styles) => {
      const { ownerState } = props;

      return [
        styles.root,
        // to remain existing behavior
        styles[`maxWidth${capitalize(String(ownerState.maxWidth))}`],
        ownerState.fixed && styles.fixed,
        ownerState.disableGutters && styles.disableGutters,
      ];
    },
  }),
  useThemeProps: inProps => useThemeProps({ props: inProps, name: 'MuiContainer', defaultTheme }),
})

// Joy UI
const Container = createContainer({
  createStyledComponent: styled('div', {
    name: 'JoyContainer',
    slot: 'Root',
    overridesResolver: (props, styles) => styles.root,
  }),
  useThemeProps: inProps => useThemeProps({ props: inProps, name: 'JoyContainer' }),
})

createBox should follow the same but we can refactor later

Move className generation to another package

I think it is quite clear that the classNames generation deserves its own package. It is not a part of any packages (Base, system, ...etc). We can keep it as internal package and use it as a dependency or we can put it inside @mui/utils. (I rather goes with its own package @mui/class-name.

I think this is not a big afford and does not introduce breaking change. However, happy to move this to another PR which should be merged before this Container PR.

@mnajdova
Copy link
Member Author

Folder structure:

I think the create... API should stay inside its folder. Ex:

Agree, makes sense to me 👍

createContainer API

I think the responsibility of the system component creator is to provide props + classNames + styles to a styled component function.

Makes sense, I didn't envision that the components would have different names, but if this is used in userland, it may be a valid use-case. Will update the PR.

Move className generation to another package

I think this is not a big afford and does not introduce breaking change. However, happy to move this to another PR which should be merged before this Container PR.

As the base package is not stable, we could move the classes utils to a different package, but I would export them still from base, at least for one release, so that we give a chance to the X team to update their imports paths (and we would need to do the same in the Mateiral UI (and Joy UI?) components. (I will draft a PR for this today)

@siriwatknp
Copy link
Member

siriwatknp commented Apr 26, 2022

As the base package is not stable, we could move the classes utils to a different package, but I would export them still from base, at least for one release, so that we give a chance to the X team to update their imports paths (and we would need to do the same in the Mateiral UI (and Joy UI?) components. (I will draft a PR for this today)

I think we should reexport them from all packages, Material UI, Joy, System, Base. Users should not know about this classname internal package (let's say that it is for internal usage between packages for now).

These are what I think should be inside the package:

  • composeClasses
  • generateUtilityClass
  • generateUtilityClasses
  • ClassNameGenerator

Comment on lines 9 to 12
/**
* Override or extend the styles applied to the component.
*/
classes?: Partial<ContainerClasses>;
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
/**
* Override or extend the styles applied to the component.
*/
classes?: Partial<ContainerClasses>;

Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need classes anymore (at least for Joy UI)

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Great work!

Comment on lines 9 to 12
/**
* Override or extend the styles applied to the component.
*/
classes?: Partial<ContainerClasses>;
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need classes anymore (at least for Joy UI)

...(!ownerState.disableGutters && {
paddingLeft: theme.spacing(2),
paddingRight: theme.spacing(2),
// @ts-ignore module augmentation fails if custom breakpoints are used
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 had to add bunch of @ts-ignore when using breakpoints, as we have a module augmentation test that fails as these breakpoints do not exits 😅 Not the best, but I don't have a better option for now...

@mnajdova mnajdova merged commit 41a5f13 into mui:master May 11, 2022
@mnajdova mnajdova changed the title [system] Add Container and re-export from other packages [system] Add Container component and createContainer factory May 11, 2022
@mnajdova mnajdova changed the title [system] Add Container component and createContainer factory [system] Add Container component and createContainer factory May 11, 2022
@mnajdova mnajdova removed the on hold There is a blocker, we need to wait label May 11, 2022
Comment on lines +15 to +21
return [
styles.root,
styles[`maxWidth${capitalize(String(ownerState.maxWidth))}`],
ownerState.fixed && styles.fixed,
ownerState.disableGutters && styles.disableGutters,
];
},
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic only here temporarily until v6? Should we have a TODO comment so we don't forget to remove it at some point?

Copy link
Member Author

Choose a reason for hiding this comment

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

The point here is that for Joy and Material UI the overrides resolver are different. In Joy, we do not have style overrides with keys, it is a leftover from Material UI. I will add a comment for updating this in v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Container The React component package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants