-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Popover] Doesn't reposition with anchorEl #7479
Conversation
src/internal/Popover.js
Outdated
@@ -281,6 +295,17 @@ class Popover extends Component<DefaultProps, Props, void> { | |||
} | |||
}; | |||
|
|||
handleResize = debounce( | |||
() => { | |||
const element: any = ReactDOM.findDOMNode(this.transitionEl); |
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.
What would be the correct type to use?
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.
flow already know the type of ReactDOM.findDOMNode
, no need to add it.
src/internal/Popover.js
Outdated
}, | ||
166, | ||
{ | ||
leading: true, |
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.
What about the last value with leading: true. Do we have it? I feels like we don't. https://sking7.github.io/articles/1248932487.html
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 added that leading option to reduce the lag in updating the position of the popover. Would you like me remove it?
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.
Yes, If I can choose, I would rather have the last event than the first one.
src/internal/Popover.js
Outdated
@@ -437,13 +462,15 @@ class Popover extends Component<DefaultProps, Props, void> { | |||
role={role} | |||
onRequestTimeout={this.handleRequestTimeout} | |||
transitionAppear | |||
ref={el => (this.transitionEl = el)} |
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 use the wording node
on the rest of the codebase for a ref callback argument. Can we stick to that convention?
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 taking care of the issue!
No problem. Changes done! |
@quiaro First merged PR here 🎉 |
Closes #5937