korbit-ai[bot] commented on code in PR #34201:
URL: https://github.com/apache/superset/pull/34201#discussion_r2212467498
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -70,9 +70,44 @@ export default function downloadAsImage(
return true;
};
+ // Check if element is scrollable
+ const isScrollable = hasScrollableDescendant(elementToPrint as
HTMLElement);
+ let targetElement = elementToPrint as HTMLElement;
+ let cloned: HTMLElement | null = null;
+ const extraPadding = 50;
+
+ if (isScrollable) {
+ // Apply styles to cloned element
+ cloned = elementToPrint.cloneNode(true) as HTMLElement;
+ cloned.style.overflow = 'visible';
+ cloned.style.maxHeight = 'none';
+ cloned.style.height = 'auto';
+ cloned.style.width = `${elementToPrint.scrollWidth}px`;
+
+ // Make sure all children don't clip
+ cloned.querySelectorAll<HTMLElement>('*').forEach(el => {
+ // eslint-disable-next-line no-param-reassign
+ el.style.overflow = 'visible';
+ // eslint-disable-next-line no-param-reassign
+ el.style.maxHeight = 'none';
+ // eslint-disable-next-line no-param-reassign
+ el.style.height = 'auto';
+ });
Review Comment:
### Noisy Style Assignments <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Repetitive style assignments with multiple eslint-disable comments make the
code harder to read.
###### Why this matters
The repeated eslint-disable comments and similar style assignments create
visual noise and make the code more difficult to scan and understand.
###### Suggested change ∙ *Feature Preview*
Extract the style assignments into a function:
```typescript
const applyNonClippingStyles = (element: HTMLElement) => {
Object.assign(element.style, {
overflow: 'visible',
maxHeight: 'none',
height: 'auto',
});
};
// Make sure all children don't clip
cloned.querySelectorAll<HTMLElement>('*').forEach(applyNonClippingStyles);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f541ab38-d355-4ae1-bee7-4c13ee005f78/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f541ab38-d355-4ae1-bee7-4c13ee005f78?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f541ab38-d355-4ae1-bee7-4c13ee005f78?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f541ab38-d355-4ae1-bee7-4c13ee005f78?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f541ab38-d355-4ae1-bee7-4c13ee005f78)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d9ac59ae-7a67-41d9-8bb4-aee0f01ebdca -->
[](d9ac59ae-7a67-41d9-8bb4-aee0f01ebdca)
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -83,6 +118,38 @@ export default function downloadAsImage(
})
.catch((e: Error) => {
console.error('Creating image failed', e);
+ })
+ .finally(() => {
+ if (cloned) {
+ document.body.removeChild(cloned);
+ }
});
};
}
+
+/**
+ * Check if an element or any of its child elements is scrollable
+ *
+ * @param el - The HTMLElement to check for scrollable descendants.
+ * @returns `true` if any descendant has scrollable overflow; otherwise
`false`.
+ */
+function hasScrollableDescendant(el: HTMLElement): boolean {
+ const treeWalker = document.createTreeWalker(el, NodeFilter.SHOW_ELEMENT);
+ let node = treeWalker.nextNode();
+
+ while (node) {
+ const element = node as HTMLElement;
+
+ // Skip if element is .header-controls or inside .header-controls
+ if (element.closest('.header-controls')) {
+ node = treeWalker.nextNode();
+ continue;
+ }
+
+ if (element.scrollHeight > element.clientHeight) {
+ return true;
+ }
Review Comment:
### Incomplete scrollable element detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The scrollable detection logic only checks for vertical scrolling
(scrollHeight > clientHeight) but doesn't account for horizontal scrolling
(scrollWidth > clientWidth).
###### Why this matters
Charts or elements that only scroll horizontally won't be detected as
scrollable, leading to incomplete captures in the exported image.
###### Suggested change ∙ *Feature Preview*
Modify the check to account for both vertical and horizontal scrolling:
```typescript
if (element.scrollHeight > element.clientHeight ||
element.scrollWidth > element.clientWidth) {
return true;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31e792bd-bf70-4968-8654-cf97af7df335/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31e792bd-bf70-4968-8654-cf97af7df335?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31e792bd-bf70-4968-8654-cf97af7df335?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31e792bd-bf70-4968-8654-cf97af7df335?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31e792bd-bf70-4968-8654-cf97af7df335)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:85772ecf-5f26-4c57-aed3-4d661c74b361 -->
[](85772ecf-5f26-4c57-aed3-4d661c74b361)
--
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]
