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

[Button][material-next] Fix some event handlers being ignored #37647

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jun 19, 2023

Fixes: #37645

Four event handlers were not being handled in getRippleHandlers, so those were lost:

  • onClick
  • onFocus
  • onKeyUp
  • onKeyDown

I also added tests for those events.

@DiegoAndai DiegoAndai added component: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material v6.x labels Jun 19, 2023
@DiegoAndai DiegoAndai requested a review from mnajdova June 19, 2023 20:40
@DiegoAndai DiegoAndai self-assigned this Jun 19, 2023
@DiegoAndai
Copy link
Member Author

Let me know if you think we should add other events to the test.

@mui-bot
Copy link

mui-bot commented Jun 19, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against bbe3641

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jul 5, 2023

Hey @mnajdova! Thanks for the feedback. I refactored the fix to use useSlotProps

The test_static test is failing because yarn proptypes wants to remove the four event handlers (onClick, onFocus, onKeyUp, onKeyDown) from the Button component's proptypes 🤔 I think that's incorrect, they shouldn't be removed, but I'm not sure, what do you think? Should the inherited props from button be included in the Button component proptypes?

@DiegoAndai DiegoAndai requested a review from mnajdova July 5, 2023 14:29
@mnajdova mnajdova requested a review from michaldudak July 7, 2023 19:05
@mnajdova
Copy link
Member

mnajdova commented Jul 7, 2023

The test_static test is failing because yarn proptypes wants to remove the four event handlers (onClick, onFocus, onKeyUp, onKeyDown) from the Button component's proptypes 🤔 I think that's incorrect, they shouldn't be removed, but I'm not sure, what do you think? Should the inherited props from button be included in the Button component proptypes?

Well this is somewhat expected as we are not handling them in the component, but they are handled in the hook. @michaldudak what are we doing with these use-cases in Base UI?

@michaldudak
Copy link
Member

These event handlers are common to all HTML elements, so they are not explicitly listed in Button or useButton. Also, they are not directly used by the hook (or the component), so essentially, they are not needed (although it does feel weird that onClick isn't defined on a button).

@mnajdova
Copy link
Member

although it does feel weird that onClick isn't defined on a button

We can discuss this separately

@DiegoAndai DiegoAndai merged commit 1a71391 into mui:master Jul 12, 2023
@DiegoAndai DiegoAndai deleted the material-next-button-on-click-bug branch July 12, 2023 14:44
@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: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material v7.x
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

[Button][material-next] Some event handlers are lost
5 participants