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

Convert Material UI components to slots #8059

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

siriwatknp
Copy link
Member

No description provided.

@mui-bot
Copy link

mui-bot commented Feb 27, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8059--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 713.7 1,056.2 799.8 907.26 128.729
Sort 100k rows ms 776.9 1,302.4 1,204.6 1,067.82 179.824
Select 100k rows ms 299.4 361.6 315.8 327.1 25.579
Deselect 100k rows ms 162 306.9 302.4 267.28 55.627

Generated by 🚫 dangerJS against 81d90a9

* The custom MenuItem component used in the grid.
* @default MenuItem
*/
BaseMenuItem: React.JSXElementConstructor<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Nice!
I've extracted MenuItems that were used as Select options to the BaseSelectOption slot in #8072

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 3, 2023
@siriwatknp siriwatknp force-pushed the extract-material-ui-components branch from ed76384 to 8a7bda8 Compare March 3, 2023 08:57
Comment on lines -3 to -5
import { IconButtonProps } from '@mui/material/IconButton';
import MenuItem, { MenuItemProps } from '@mui/material/MenuItem';
import ListItemIcon from '@mui/material/ListItemIcon';
Copy link
Member Author

Choose a reason for hiding this comment

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

@cherniavskii For the type's usage from Material UI, what do you think about this approach?

Create a generic type, so that this component is design-agnostic. And at the index.ts, fill the generic with the types from Material or Joy UI.

Developer that import { GridActionsCellItem } from '@mui-x/data-grid' will still get the same types from Material UI.

Developer that import { GridActionsCellItem } from '@mui-x/data-grid/joy' will get types from Joy UI.

Comment on lines +1 to +13
import { IconButtonProps } from '@mui/material/IconButton';
import { MenuItemProps } from '@mui/material/MenuItem';

import {
GridActionsCellItem as InternalGridActionsCellItem,
GridActionsCellItemProps as InternalActionsCellItemProps,
} from './GridActionsCellItem';

export type GridActionsCellItemProps = InternalActionsCellItemProps<IconButtonProps, MenuItemProps>;
export const GridActionsCellItem = InternalGridActionsCellItem as React.ForwardRefExoticComponent<
React.PropsWithoutRef<GridActionsCellItemProps> & React.RefAttributes<HTMLButtonElement>
>;

Copy link
Member Author

Choose a reason for hiding this comment

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

Here, the types of GridActionsCellItem is inserted by Material UI component props.

Comment on lines +1 to +2
import { IconButtonProps } from '@mui/joy/IconButton';
import { MenuItemProps } from '@mui/joy/MenuItem';
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
import { IconButtonProps } from '@mui/joy/IconButton';
import { MenuItemProps } from '@mui/joy/MenuItem';
import type { IconButtonProps } from '@mui/joy/IconButton';
import type { MenuItemProps } from '@mui/joy/MenuItem';

Comment on lines +4 to +7
import {
GridActionsCellItem as InternalGridActionsCellItem,
GridActionsCellItemProps as InternalActionsCellItemProps,
} from '../../../components/cell/GridActionsCellItem';
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
import {
GridActionsCellItem as InternalGridActionsCellItem,
GridActionsCellItemProps as InternalActionsCellItemProps,
} from '../../../components/cell/GridActionsCellItem';
import { GridActionsCellItem as InternalGridActionsCellItem } from '../../../components/cell/GridActionsCellItem';
import type { GridActionsCellItemProps as InternalActionsCellItemProps } from from '../../../components/cell/GridActionsCellItem';

@cherniavskii cherniavskii self-requested a review April 27, 2023 09:32
@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label May 18, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants