Antonio-RiveroMartnez commented on code in PR #36051:
URL: https://github.com/apache/superset/pull/36051#discussion_r2510001005
##########
superset/utils/screenshot_utils.py:
##########
@@ -121,62 +121,52 @@ def take_tiled_screenshot(
)
# Calculate number of tiles needed
- num_tiles = max(1, (dashboard_height + viewport_height - 1) //
viewport_height)
+ num_tiles = max(1, (dashboard_height + tile_height - 1) // tile_height)
logger.info("Taking %s screenshot tiles", num_tiles)
screenshot_tiles = []
for i in range(num_tiles):
# Calculate scroll position to show this tile's content
- scroll_y = dashboard_top + (i * viewport_height)
-
- # Scroll the window to the desired position
- page.evaluate(f"window.scrollTo(0, {scroll_y})")
- logger.debug(
- "Scrolled window to %s for tile %s/%s", scroll_y, i + 1,
num_tiles
- )
+ y = dashboard_top + (i * tile_height)
Review Comment:
nit: I preferred the larger names on these variables, i.e, `y` and `x`
probably not as readable.
##########
superset/utils/screenshot_utils.py:
##########
@@ -121,62 +121,52 @@ def take_tiled_screenshot(
)
# Calculate number of tiles needed
- num_tiles = max(1, (dashboard_height + viewport_height - 1) //
viewport_height)
+ num_tiles = max(1, (dashboard_height + tile_height - 1) // tile_height)
logger.info("Taking %s screenshot tiles", num_tiles)
screenshot_tiles = []
for i in range(num_tiles):
# Calculate scroll position to show this tile's content
- scroll_y = dashboard_top + (i * viewport_height)
-
- # Scroll the window to the desired position
- page.evaluate(f"window.scrollTo(0, {scroll_y})")
- logger.debug(
- "Scrolled window to %s for tile %s/%s", scroll_y, i + 1,
num_tiles
- )
+ y = dashboard_top + (i * tile_height)
+ x = dashboard_left
+ page.evaluate(f"window.scrollTo(0, {y})")
+ logger.debug("Scrolled window to %s for tile %s/%s", y, i + 1,
num_tiles)
# Wait for scroll to settle and content to load
- page.wait_for_timeout(2000) # 2 second wait per tile
-
- # Get the current element position after scroll
- current_element_box = page.evaluate(f"""() => {{
- const el = document.querySelector(".{element_name}");
- const rect = el.getBoundingClientRect();
- return {{
- x: rect.left,
- y: rect.top,
- width: rect.width,
- height: rect.height
- }};
- }}""")
+ page.wait_for_timeout(1000)
Review Comment:
nit: I know this code is like this from before, but should we make that
timeout a const at the top? so we can change/play with it later if needed
without having o chase it down here?
##########
superset/utils/screenshot_utils.py:
##########
@@ -190,9 +180,6 @@ def take_tiled_screenshot(
logger.info("Combining screenshot tiles...")
combined_screenshot = combine_screenshot_tiles(screenshot_tiles)
- # Reset window scroll position
- page.evaluate("window.scrollTo(0, 0)")
Review Comment:
is this intended?
--
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]
