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

[LoadingButton] Fix padding of loading icon in small button #31113

Merged
merged 7 commits into from Mar 7, 2022
Merged

[LoadingButton] Fix padding of loading icon in small button #31113

merged 7 commits into from Mar 7, 2022

Conversation

PunitSoniME
Copy link
Contributor

@PunitSoniME PunitSoniME commented Feb 16, 2022

Closes #31109

@mui-bot
Copy link

mui-bot commented Feb 16, 2022

Details of bundle changes

Generated by 🚫 dangerJS against fe667a6

@danilo-leal danilo-leal changed the title - fix - Padding of loading icon in small button [Loading Button] Fix padding of loading icon in small button Feb 16, 2022
@danilo-leal danilo-leal added component: LoadingButton The React component. new feature New feature or request labels Feb 16, 2022
@hbjORbj hbjORbj changed the title [Loading Button] Fix padding of loading icon in small button [LoadingButton] Fix padding of loading icon in small button Feb 16, 2022
@sjdemartini
Copy link
Contributor

Thanks for tackling this PunitSoniME! Is this change ready to merge? Definitely would appreciate the bug fix being released!

@PunitSoniME
Copy link
Contributor Author

Thanks for tackling this PunitSoniME! Is this change ready to merge? Definitely would appreciate the bug fix being released!

@sjdemartini Yes it is ready to merge

@PunitSoniME
Copy link
Contributor Author

@sjdemartini

Can you please merge this PR ?

@sjdemartini
Copy link
Contributor

@PunitSoniME I'm not a maintainer unfortunately, just the person who filed the original bug report. @hbjORbj would you be able to merge this in? Thanks!

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.

It should be this, otherwise <LoadingButton size="small" variant="text"> will cause a regression.

{
  ...(ownerState.loadingPosition === 'start' &&
    (ownerState.variant === 'outlined' || ownerState.variant === 'contained') && {
      left: ownerState.size === 'small' ? 10 : 14,
    }),
 ...(ownerState.loadingPosition === 'end' &&
    (ownerState.variant === 'outlined' || ownerState.variant === 'contained') && {
      right: ownerState.size === 'small' ? 10 : 14,
    }),
}

Also, can you add size small to the loading demo so that we have the regression test.

@hbjORbj
Copy link
Member

hbjORbj commented Feb 28, 2022

@sjdemartini Hey, sure, we can merge after tests are written.

@PunitSoniME Would you add unit tests for the change you made? You can add them to this file: https://github.com/mui/material-ui/blob/master/packages/mui-lab/src/LoadingButton/LoadingButton.test.js

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! labels Feb 28, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 28, 2022

Would you add unit tests for the change you made?

Would a visual test be more effective?

@siriwatknp
Copy link
Member

Would you add unit tests for the change you made?

Would a visual test be more effective?

adding a small button to the loading demo should be enough.

@PunitSoniME
Copy link
Contributor Author

@siriwatknp @oliviertassinari @hbjORbj
Small button demo added for button component

image

I hope this would be enough to merge.

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 for the fix! I updated the demo to have initial loading so that we can track the visual regression.

@siriwatknp siriwatknp merged commit c11bbd7 into mui:master Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! component: LoadingButton The React component. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoadingButton] loading indicator in start/end position is misaligned when button size="small"
7 participants