codeant-ai-for-open-source[bot] commented on PR #38535:
URL: https://github.com/apache/superset/pull/38535#issuecomment-4906958799
Question: - **dropdown overlay left permanently hidden on capture failure**
(@codeant-ai-for-open-source, @rusackas) **+ z-index stacking context**
(@copilot, @alexandrusoare) **+ "is this needed?"** (@msyavuz): removed the
custom `dropdownoverlaycontainerref` popup container entirely and reverted to
master's `.ant-dropdown:not(.ant-dropdown-hidden)` visibility hack. all three
handlers now restore visibility in a `.finally()` so a failed screenshot/pdf
can't leave the menu invisible.
- **duplicated png/pdf export code + hard-coded selector** (@sadpandajoe,
@copilot, @rusackas): parameterized `getexportscreenshotmenuitems` (menu keys)
and replaced the second menu's inline png submenu + pdf item with a spread of
the helper; both call sites now use `chart_export_selector` (including the jpeg
handlers).
- **missing log-action registration** (@sadpandajoe): added
`log_actions_chart_download_as_png` / `_as_pdf` to `log_event_type_user`.
- **missing tests** (@sadpandajoe): exported and unit-tested
`getexportscreenshotmenuitems`, and added the solid-background counterpart test
in `sliceheadercontrols.test.tsx`. png labels already read `export screenshot
(png)`.
per @rusackas, the `alertreportmodal.test.tsx` `queryallbytext` tightening
suggestions were left as pre-existing / out of scope.
Answer:
Here’s my read on the PR questions:
- **Dropdown overlay left permanently hidden on capture failure / z-index
stacking context / “is this needed?”**
Yes — the current implementation now restores the dropdown visibility in a
`.finally()` for all three paths (jpeg, PNG, PDF), so a failed capture won’t
leave the menu hidden.
Also, removing the custom popup container and going back to the
`.ant-dropdown:not(.ant-dropdown-hidden)` visibility toggle seems reasonable
here; it keeps the fix localized and avoids the z-index/container complexity.
I agree the previous custom container approach was probably overkill.
- **Duplicated PNG/PDF export code + hard-coded selector**
Yes — this has been cleaned up well. `getExportScreenshotMenuItems(...)`
centralizes the PNG/PDF submenu construction, and both menu entry points now
reuse it.
The chart selector is also now a shared constant (`CHART_EXPORT_SELECTOR`)
instead of being duplicated inline, which makes the jpeg/png/pdf paths
consistent.
- **Missing log-action registration**
Fixed. `LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG` and
`LOG_ACTIONS_CHART_DOWNLOAD_AS_PDF` are now included in `LOG_EVENT_TYPE_USER`,
so those events will be categorized correctly.
- **Missing tests**
Also addressed:
- `getExportScreenshotMenuItems` is exported and unit-tested.
- Dashboard slice header export paths are covered, including the PNG
solid/transparent options and PDF.
- The added tests verify both the download function calls and logging
behavior.
- **About the `AlertReportModal.test.tsx` changes**
Agreed — those look unrelated to this feature and can be treated as
pre-existing / out of scope for this PR.
One small note: the new PNG scaling logic is a good step for quality, but it
would be worth sanity-checking that `dom-to-image` handles the
`style.transform: scale(2)` + doubled width/height combination consistently
across all chart types. If CI/manual testing already covered it, then this
looks fine.
--
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]
