codeant-ai-for-open-source[bot] commented on code in PR #37978:
URL: https://github.com/apache/superset/pull/37978#discussion_r3558801043
##########
superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx:
##########
@@ -140,7 +149,15 @@ export const ModalTrigger = forwardRef(
draggableConfig={draggableConfig}
destroyOnHidden={destroyOnHidden}
>
- {modalBody}
+ <div
+ onKeyDown={e => {
+ if (MENU_NAVIGATION_KEYS.has(e.key)) {
+ e.stopPropagation();
+ }
+ }}
+ >
+ {modalBody}
+ </div>
Review Comment:
**Suggestion:** The new keyboard boundary wrapper introduces an extra
block-level container with default auto height, which changes modal body
layout. Several existing modal bodies (especially resizable ones) rely on being
the direct 100%-height child of the modal body, so this wrapper can cause
content to stop filling available height and regress scrolling/resizing
behavior. Keep the keyboard boundary but make this wrapper layout-neutral (or
explicitly full-height) so existing modal content sizing contracts remain
intact. [css layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart data modal no longer fills resizable viewport.
- ❌ View query modal height no longer matches modal body.
- ⚠️ Resizing dashboard modals yields cramped, partially visible table
content.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard and use the slice header controls menu (component
`SliceHeaderControls` at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:252-236`)
to
click **“View as table”**, which renders `ViewResultsModalTrigger` at
`ViewResultsModalTrigger.tsx:47-91`.
2. `ViewResultsModalTrigger` invokes `ModalTrigger` with `responsive`,
`resizable`, and a
custom `resizableConfig` (file
`superset-frontend/src/dashboard/components/SliceHeaderControls/ViewResultsModalTrigger.tsx:47-61`),
passing `modalBody` as a `ResultsPaneOnDashboard` component.
3. Inside `ModalTrigger`
(`superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx:118-144`),
the new keyboard boundary now wraps `modalBody` in an extra `<div>` at lines
135-143, so
the DOM becomes `.ant-modal-body > keyboardBoundaryDiv > Wrapper > Tabs...`.
4. `ResultsPaneOnDashboard` defines its root `Wrapper` with `height: 100%`
and makes
nested AntD tabs and tab panes fill height
(`superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx:25-41`),
relying on being the direct child of `.ant-modal-body` whose height is
explicitly set for
resizable modals in `Modal.tsx`
(`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:185-203`).
5. Because the new keyboardBoundary `<div>` has default `height: auto`, the
`Wrapper`’s
`height: 100%` no longer references the modal body but this auto-height
parent, causing
the results table area to no longer fill the resizable viewport: vertical
space is
constrained, scrolling area is reduced, and resizing the modal body does not
fully stretch
the content.
6. The same layout issue affects the resizable “View query” modal launched
from
SliceHeaderControls
(`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:99-115,
99-115`
where a `ModalTrigger` with `draggable`, `resizable`, `responsive` wraps
`ViewQueryModal`
whose container uses `height: 100%` at `ViewQueryModal.tsx:44-49`),
producing truncated
content and inconsistent scroll behavior in normal usage.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3d5d34a54e924cb9adad8fb8ef4f0e62&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3d5d34a54e924cb9adad8fb8ef4f0e62&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx
**Line:** 152:160
**Comment:**
*Css Layout Issue: The new keyboard boundary wrapper introduces an
extra block-level container with default auto height, which changes modal body
layout. Several existing modal bodies (especially resizable ones) rely on being
the direct 100%-height child of the modal body, so this wrapper can cause
content to stop filling available height and regress scrolling/resizing
behavior. Keep the keyboard boundary but make this wrapper layout-neutral (or
explicitly full-height) so existing modal content sizing contracts remain
intact.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37978&comment_hash=7f0f3b27e3ecbf220ae43df1915d32354493906f98d9f566e37c0b3356c69d6c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37978&comment_hash=7f0f3b27e3ecbf220ae43df1915d32354493906f98d9f566e37c0b3356c69d6c&reaction=dislike'>👎</a>
--
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]
