-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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-ui][Popover] Allow null
in anchorEl
function return type
#45045
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: eduter <[email protected]>
Netlify deploy previewhttps://deploy-preview-45045--material-ui.netlify.app/ Bundle size report |
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.
@eduter Why not use state
to set the anchor element when the button is clicked, as shown in the Popover docs: https://mui.com/material-ui/react-popover/#basic-popover? This avoids React rule violations since state updates are idempotent.
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.
Thanks for the contribution @eduter!
Why not use state to set the anchor element when the button is clicked, as shown in the Popover docs: https://mui.com/material-ui/react-popover/#basic-popover?
I agree that following the docs is the way to go.
That said, this change does make sense to me. The function's returning null would be valid as well.
The only section of the code in which this type change might have unintended consequences is:
const containerWindow = ownerWindow(anchorEl); |
As anchorEl
is used directly, and not through resolveAnchorEl
. I think this line might be incorrect. @eduter may I ask you to investigate the history a bit and see if there's a reason not to use the resolved anchor element there?
@eduter may I ask you to add a spec test for this updated type?
null
in anchorEl
function return type
@DiegoAndai I took over this PR since the author hasn’t responded.
It was added in #22159. Resolving the anchor element function was already introduced until then. I think this was just missed. A separate PR would be better for this fix. What do you think?
I added it. I looked into why |
I'm using a ref for a button as the
anchorEl
, but this forces me to, either:ref.current
during the rendering phase, breaking one of the Rules of React, about idempotency and making the React Compiler bail on my component.Looking at the implementation of resolveAnchorEl, I can see that, in practice, it doesn't matter if the null was passed directly or if it's the return value of a function. The type of the prop is inconsistent with the implementation.
This is how I ran into this problem: