-
Notifications
You must be signed in to change notification settings - Fork 1.2k
✨ [Popover] Now observes overlay attributes #4372
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
Conversation
size-limit report
|
kyledurand
left a comment
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.
Love this approach and that we don't have to add another nested prop to popover 👍
Thanks @beefchimi
| mutationObserverObserveSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('observers the overlay', () => { |
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.
| it('observers the overlay', () => { | |
| it('observes the overlay', () => { |
822e278 to
4c53e60
Compare
AndrewMusgrave
left a comment
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.
LGTM! ✅
4174186 to
c2c2ac1
Compare
c2c2ac1 to
c033567
Compare
|
Closed in favour of: #4385 |
This PR replaces the previous effort: #4303
We are now always observing the Overlay
attributes. We feel that the performance cost will not be that severe, and it is better to take this first step than to introduce another prop to the API.Our main ask here is having control over how the
Popoverre-renders (re-sizes/positions itself). Eventually, we think a better solution is to offer a mechanism to force a re-render. In the words of @clauderic