rebenitez1802 commented on code in PR #44313:
URL: https://github.com/apache/superset/pull/44313#discussion_r4029671007
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -46,10 +46,55 @@ type EmbeddedSupersetApi = {
setDataMask: ({ dataMask }: { dataMask: DataMaskStateWithId }) => void;
};
-const getScrollSize = (): Size => ({
- width: document.body.scrollWidth,
- height: document.body.scrollHeight,
-});
+// Hosts size the iframe from this value, so it has to describe the content and
+// not the frame the host already set. Lift the fill-the-frame constraints,
read,
+// restore, all in one synchronous task. The styles are set inline so they win
+// the cascade outright: a rule that silently loses it turns this back into a
+// measurement of the frame.
+const getScrollSize = (): Size => {
+ const root = document.documentElement;
+ const { body } = document;
+ const app = document.getElementById('app');
+ const content = document.querySelector('#app .dashboard');
+
+ // Nothing has laid out yet, so report the viewport. A host applying a near
+ // zero height would collapse the frame, and charts render only once they are
+ // in view, so the embed could not recover.
+ if (!content || content.getBoundingClientRect().height === 0) {
Review Comment:
Non-blocking robustness nit: this "nothing has laid out yet" guard only
checks that `.dashboard` has non-zero height. But a mounted `.dashboard`
already has height from its header/tabs while below-the-fold charts are still
lazy-loading. If a host calls `getScrollSize` in that window, the guard is
bypassed and `body.scrollHeight` reflects only the short pre-render layout —
the host then shrinks the iframe, the below-fold charts never enter the
viewport, never render, and the embed can't recover. That's the exact failure
this guard warns about, just triggered by *partial* layout instead of *zero*
layout. Could be worth gating on a readiness signal rather than height alone.
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -46,10 +46,55 @@ type EmbeddedSupersetApi = {
setDataMask: ({ dataMask }: { dataMask: DataMaskStateWithId }) => void;
};
-const getScrollSize = (): Size => ({
- width: document.body.scrollWidth,
- height: document.body.scrollHeight,
-});
+// Hosts size the iframe from this value, so it has to describe the content and
+// not the frame the host already set. Lift the fill-the-frame constraints,
read,
+// restore, all in one synchronous task. The styles are set inline so they win
+// the cascade outright: a rule that silently loses it turns this back into a
+// measurement of the frame.
+const getScrollSize = (): Size => {
+ const root = document.documentElement;
+ const { body } = document;
+ const app = document.getElementById('app');
+ const content = document.querySelector('#app .dashboard');
+
+ // Nothing has laid out yet, so report the viewport. A host applying a near
+ // zero height would collapse the frame, and charts render only once they are
+ // in view, so the embed could not recover.
+ if (!content || content.getBoundingClientRect().height === 0) {
+ return { width: body.scrollWidth, height: root.clientHeight };
+ }
+
+ // The bounded filter bar is capped to the frame as well, so the cap comes
off
+ // too or a tall filter list reports as frame-high.
+ const bar = document.querySelector<HTMLElement>('.filter-bar-bounded');
+ const scroller = bar?.querySelector<HTMLElement>('.filter-bar-scroll');
Review Comment:
Non-blocking maintainability nit: this couples to the `.filter-bar-bounded`
/ `.filter-bar-scroll` class names emitted by `Vertical.tsx`, with no
compile-time link. `api.test.ts` builds its own fixture DOM using those same
string literals, so renaming the class in `Vertical.tsx` would silently regress
production (scroll position no longer captured/restored on host resize) while
the unit test stays green. A shared constant for the class names — or a test
that renders the real component instead of a hand-built fixture — would make
the coupling visible to CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
