Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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][base] Drop
component
prop #36677[Button][base] Drop
component
prop #36677Changes from 11 commits
c955845
8e7ebda
615cfaf
3b88d2d
42c3a54
abb7d98
7eecd8a
aced6ee
b96cc6f
1aca46b
ee4fdb7
f49956d
c709d0c
d61f431
d86107a
e0cc64d
10cca14
83aaa1e
aa50cdc
0c78312
5c8d684
641bd26
27e0da1
cb62291
4ebe89f
41845e6
814df7c
253450a
8c7a61e
920a682
508a008
2bb9245
fbfbd3c
9ce855e
e089918
b7d62fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the ButtonUnstyled definition shouldn't be
function ButtonUnstyled(props: ButtonUnstyledProps, forwardedRef: React.ForwardedRef<any>)
(without the generics). This way, we get strongly typed props inside the component definition. Nothing would change with the exported definition as we cast it to PloymorphicComponent anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the explicitness that we have now. We can open a separate issue for this and see what other things I guess. It's anyway internal change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaldudak I think we should go the other way around and explicitly define these props in the ButtonUnstyledProps interface, as if a custom component is used, that does not have these props in the props interface, there will be a typescript error, which would be a bug in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand you correctly. Why would there be a TS error? We don't actually use these props anywhere. Also in ButtonUnstyled.spec.tsx we have a case with
CustomComponent
that don't have these defined and it works fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have with custom component. For e.g. this is failing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I get it now. It's an interesting problem. The
ButtonUnstyled
doesn't handle (or require) the click (or several other) events, so strictly speaking, it's unnecessary. It's necessary in theslots.root
, though, but there is no way of enforcing it.In other words, even if we had
onClick
inButtonUnstyledProps
, it would be possible to provideCustomComponent
that ignores itsonClick
prop and effectively create a broken button.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a failing test: https://app.circleci.com/pipelines/github/mui/material-ui/95773/workflows/1ec2c63d-7103-4340-b894-909bf873b7a0/jobs/509209
I think we should update the types to add these event handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of thoughts I have about this:
MouseEventHandler<Element>
orMouseEventHandler<HTMLButtonElement>
. In either case, another test will fail (the last one from the .spec suite).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. I've added ts-expect-error directive on the new test, so that in the future, if we decide to handle this, we have some record that it was an intended decision. We can discuss it separately tough in the future.