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

[joy-ui][IconButton] Support loading prop #40949

Merged
merged 4 commits into from Feb 12, 2024

Conversation

Smileek
Copy link
Contributor

@Smileek Smileek commented Feb 5, 2024

This closes #36692

Mostly stolen from the Button component, but seemed to be a good task to start getting to know the project.

@mui-bot
Copy link

mui-bot commented Feb 5, 2024

Netlify deploy preview

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

@mui/joy: parsed: +0.17% , gzip: +0.20%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5cc23ad

@Smileek Smileek force-pushed the smileek/icon-button-loading branch 2 times, most recently from 340fc29 to 420c3a1 Compare February 5, 2024 16:10
@Smileek
Copy link
Contributor Author

Smileek commented Feb 5, 2024

In the meantime, fixed a bug in the Button component:

image

ButtonGroup's disabled prop wasn't overridden, because the first part of (inProps.disabled || inProps.loading) ?? (buttonGroup.disabled || disabledProp || loading); resulted in (false || undefined) which is undefined.

@Smileek Smileek marked this pull request as ready for review February 5, 2024 17:29
@zannager zannager added component: icon button This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Feb 6, 2024
@ZeeshanTamboli ZeeshanTamboli added the new feature New feature or request label Feb 7, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@Smileek, thanks for the pull request.

In the meantime, fixed a bug in the Button component:

image

ButtonGroup's disabled prop wasn't overridden, because the first part of (inProps.disabled || inProps.loading) ?? (buttonGroup.disabled || disabledProp || loading); resulted in (false || undefined) which is undefined.

Could you address this in a separate PR and undo the changes related to this in the current PR?


Before reviewing the code, could you explain your use case for needing this feature? Is it necessary for your application?

@ZeeshanTamboli ZeeshanTamboli changed the title [joy-ui][IconButton] loading prop for IconButton [joy-ui][IconButton] Support loading prop Feb 8, 2024
@Smileek
Copy link
Contributor Author

Smileek commented Feb 8, 2024

@ZeeshanTamboli sure. Here's another one: #41000

@ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli sure. Here's another one: #41000

Thanks for that pull request. I have reviewed it.

@Smileek
Copy link
Contributor Author

Smileek commented Feb 9, 2024

@ZeeshanTamboli thanks for the review, everything is fixed: a4703b5

pnpm docs:build && pnpm docs:export runs well locally, so I believe Netlify failed for their own reason.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@Smileek Thanks for your contribution!

@ZeeshanTamboli ZeeshanTamboli merged commit 8265b03 into mui:master Feb 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon button This is the name of the generic UI component, not the React module! new feature New feature or request package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IconButton][Joy] support loading prop
4 participants