-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Ensure previews work for plots #7459
Ensure previews work for plots #7459
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7459 +/- ##
==========================================
- Coverage 55.37% 51.85% -3.53%
==========================================
Files 671 671
Lines 26920 26930 +10
Branches 2614 2614
==========================================
- Hits 14908 13964 -944
- Misses 11297 12250 +953
- Partials 715 716 +1
*This pull request uses carry forward flags. Click here to find out more.
... and 124 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -61,6 +61,7 @@ class Overlay extends EventEmitter { | |||
dismiss() { | |||
this.emit('destroy'); | |||
this.destroy(); | |||
this.container.remove(); |
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.
overlays were stacking up!
root: rootContainer, | ||
rootMargin: '0px', | ||
threshold: 1.0 | ||
root: rootContainer |
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 need defaults here (specifically the 0
threshold) so our callback runs when the whole canvas is gone, or if a single pixel of the canvas is visible. this keeps the intersection conservative in its definition of "visible"
if (!this.chartVisible) { | ||
// destroy the chart | ||
const isNowVisible = entry.isIntersecting; | ||
const chartInOverlayWindow = this.chartContainer?.closest('.js-overlay') !== null; |
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.
intersection observers in previews behave badly as the overlay covers our root element! try to detect that we're in one.
element: vNode.el, | ||
size: 'large', | ||
autoHide: false, | ||
buttons: [ | ||
{ | ||
label: 'Done', | ||
callback: () => overlay.dismiss() | ||
callback: () => { | ||
overlay.dismiss(); |
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.
overlays were stacking up!
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.
oooo nice catch
#observerCallback = ([entry]) => { | ||
if (entry.target === this.#element) { | ||
this.isIntersecting = entry.isIntersecting; | ||
if (this.#inOverlay() && !entry.isIntersecting) { |
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.
same issue as renderWhenVisible
e2e/tests/functional/plugins/displayLayout/displayLayout.e2e.spec.js
Outdated
Show resolved
Hide resolved
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.
Nice work!
Closes #7446
Closes #7535
Describe your changes:
renderWhenVisible
function always firesAll Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist