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-next][ButtonGroup] Change ButtonGroup files to ts #39794

Merged
merged 18 commits into from
Dec 6, 2023

Conversation

lhilgert9
Copy link
Contributor

@lhilgert9 lhilgert9 commented Nov 7, 2023

ButtonGroup issue: #39686
Material You umbrella issue: #29345

Changes

  • Change ButtonGroup files to TS
  • Fix ts errors

@mui-bot
Copy link

mui-bot commented Nov 7, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b79f726

@lhilgert9 lhilgert9 marked this pull request as ready for review November 7, 2023 17:58
@mj12albert mj12albert added component: ButtonGroup The React component. v6.x package: material-ui Specific to @mui/material labels Nov 8, 2023
@lhilgert9
Copy link
Contributor Author

@mnajdova I have changed the type for the OwnerState so far. We should try to adopt this way for the other components in material-next, because as far as I know this has not been done so far.

@lhilgert9
Copy link
Contributor Author

lhilgert9 commented Nov 16, 2023

@mnajdova @mj12albert We could also consider introducing a new type in @mui/types to make it easier to create the OwnerState types. An example of this would be:

type PickRequired<T, K extends keyof T> = DistributiveOmit<T, K> & {
  [P in K]-?: T[P];
};

We could introduce this in this PR or in a separate one.

Then we could use this type as follows:

import { PickRequired } from '@mui/types'

export interface ButtonGroupOwnerState
  extends PickRequired<ButtonGroupProps,
      | 'color'
      | 'disabled'
      | 'disableElevation'
      | 'disableRipple'
      | 'disableTouchRipple'
      | 'fullWidth'
      | 'orientation'
      | 'size'
      | 'variant'
  > {}

@mj12albert
Copy link
Member

mj12albert commented Nov 20, 2023

We could also consider introducing a new type in @mui/types to make it easier to create the OwnerState types

I agree this could be helpful, we could discuss and refine in a separate issue CC @DiegoAndai

We've actually kind of started making OwnerState types with Omit/Required/Pick here and there already 😅 e.g.

@mnajdova
Copy link
Member

The types look good to me 👍 I will let @DiegoAndai chime in and let his opinion lastly before we merge this. Thanks a lot for the effort @lhilgert9 🙏

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

This looks good to me 🎉

The utility type for the owner state also makes sense to me. Left a comment on that issue.

Should we merge this @mnajdova? Go ahead if you think we should, just asking as you were reviewing it originally.

@lhilgert9
Copy link
Contributor Author

@DiegoAndai @mnajdova OwnerState type is now also fixed. Now everything should be ready in this PR to merge.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, let's udpate to matest master and merge 👍

@lhilgert9
Copy link
Contributor Author

@mnajdova Done👍🏽

@DiegoAndai DiegoAndai merged commit 93fdbe0 into mui:master Dec 6, 2023
19 checks passed
@lhilgert9 lhilgert9 deleted the change-button-group-files-to-ts branch December 6, 2023 12:03
@DiegoAndai DiegoAndai added v7.x and removed v6.x labels Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonGroup The React component. package: material-ui Specific to @mui/material v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants