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][TableSortLabel] Add slots and slotProps #44728

Merged
merged 15 commits into from
Jan 3, 2025

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Dec 11, 2024

Part of #41281

This PR just adds slots and slotProps and doesn't deprecate IconComponent yet. Check this comment for why #42157 (comment)

@sai6855 sai6855 marked this pull request as draft December 11, 2024 06:54
@mui-bot
Copy link

mui-bot commented Dec 11, 2024

Netlify deploy preview

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

TableSortLabel: parsed: +2.13% , gzip: +1.89%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 6d82864

@sai6855 sai6855 requested a review from siriwatknp December 11, 2024 07:42
@sai6855 sai6855 marked this pull request as ready for review December 11, 2024 07:42
@zannager zannager added the component: table This is the name of the generic UI component, not the React module! label Dec 11, 2024
@sai6855 sai6855 added the package: material-ui Specific to @mui/material label Dec 11, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Dec 16, 2024

The PR looks good. Let's put this on hold until the discussion is resolved.

Whether the rest of the components will not support slotProps callback (and remove slotProps callback entirely in v7) or will go as is.

Feel free to continue on other components. Once we have conclusion, there should be a minor change in each PR.

Based on the decision, we are move forward as planned before by supporting callback in the slotProps.

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.

The slots and slotProps are missing the root slot

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 2, 2025

@siriwatknp Added root slot

@sai6855 sai6855 requested a review from siriwatknp January 2, 2025 07:32
*/
icon: SlotProps<
React.ElementType<React.SVGAttributes<SVGSVGElement>>,
{},
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
{},
TableSortLabelIconSlotPropsOverrides,

*/
root: SlotProps<
React.ElementType<React.HTMLAttributes<HTMLSpanElement>>,
{},
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
{},
TableSortLabelRootSlotPropsOverrides,

/**
* Props forwarded to the icon slot.
*/
icon: SlotProps<
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you move the root first (before icon). I find it easier to scan.

}
>;

export interface TableSortLabelOwnerState extends TableSortLabelProps {}
Copy link
Member

@siriwatknp siriwatknp Jan 3, 2025

Choose a reason for hiding this comment

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

Suggested change
export interface TableSortLabelOwnerState extends TableSortLabelProps {}
export interface TableSortLabelOwnerState extends TableSortLabelOwnProps {}

To prevent recursion types.


export interface TableSortLabelOwnerState extends TableSortLabelProps {}

export interface TableSortLabelOwnProps extends TableSortLabelSlotsAndSlotProps {
Copy link
Member

Choose a reason for hiding this comment

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

Moved TableSortLabelSlotsAndSlotProps to TableSortLabelTypeMap instead

Suggested change
export interface TableSortLabelOwnProps extends TableSortLabelSlotsAndSlotProps {
export interface TableSortLabelOwnProps {
export type TableSortLabelTypeMap<
  AdditionalProps = {},
  RootComponent extends React.ElementType = 'span',
> = ExtendButtonBaseTypeMap<{
-  props: AdditionalProps & TableSortLabelOwnProps;
+  props: AdditionalProps & TableSortLabelOwnProps & TableSortLabelSlotsAndSlotProps;
  defaultComponent: RootComponent;
}>;

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.

👍 Nice.

@sai6855 sai6855 merged commit 69f33fc into mui:master Jan 3, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants