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

[material-ui][Divider] Convert to support CSS extraction #41366

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Mar 5, 2024

@sai6855 sai6855 marked this pull request as draft March 5, 2024 08:32
@sai6855 sai6855 added component: divider This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/* labels Mar 5, 2024
@sai6855 sai6855 changed the title [material-ui][Divider] Migrate to pigment-css [material-ui][Divider] Convert to support CSS extraction Mar 5, 2024
@mui-bot
Copy link

mui-bot commented Mar 5, 2024

Netlify deploy preview

https://deploy-preview-41366--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 0db3b6e

@sai6855 sai6855 marked this pull request as ready for review March 5, 2024 13:32
@sai6855 sai6855 requested a review from siriwatknp March 5, 2024 13:32
@sai6855
Copy link
Contributor Author

sai6855 commented Mar 5, 2024

@siriwatknp ran following commands to render demos locally but facing following error

  1. pnpm build
  2. cd apps/pigment-next-app
  3. pnpm install && pnpm dev

image

Comment on lines 96 to 97
props: ({ ownerState }) =>
ownerState.variant === 'middle' && ownerState.orientation === 'horizontal',
Copy link
Member

@siriwatknp siriwatknp Mar 6, 2024

Choose a reason for hiding this comment

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

Suggested change
props: ({ ownerState }) =>
ownerState.variant === 'middle' && ownerState.orientation === 'horizontal',
props: { variant: 'middle', orientation: 'horizontal' },

I think object comparison should be used if possible. It's shorter and more readable.

x the others.

},
},
{
props: ({ ownerState }) => ownerState.children,
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
props: ({ ownerState }) => ownerState.children,
props: ({ ownerState }) => !!ownerState.children,

should explicitly return boolean.

Copy link
Member

@siriwatknp siriwatknp Mar 6, 2024

Choose a reason for hiding this comment

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

Note: in v7, this could be replaced with pseudo :not(:empty)

@sai6855 sai6855 requested a review from siriwatknp March 6, 2024 04:04
'&::before, &::after': {
flexDirection: 'column',
Copy link
Member

@siriwatknp siriwatknp Mar 6, 2024

Choose a reason for hiding this comment

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

@sai6855 flexDirection was not the inside psuedo selector, could you recheck if this is the correct change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps sorry, fixed in this commit 0b39bba

@siriwatknp
Copy link
Member

I encounter an error after running pnpm dev in the pigment-next-app. It's because of Divider.muiSkipListHighlight, let me find a solution.

@mnajdova mnajdova changed the base branch from master to next March 19, 2024 09:32
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.

👍 Thanks!

@siriwatknp siriwatknp merged commit 9eabadc into mui:next Mar 20, 2024
19 checks passed
pluvio72 pushed a commit to pluvio72/material-ui that referenced this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: divider This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/* v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants