codeant-ai-for-open-source[bot] commented on code in PR #36536:
URL: https://github.com/apache/superset/pull/36536#discussion_r2611290834
##########
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:
##########
@@ -55,132 +55,137 @@ export const StyledModal =
styled(BaseModal)<StyledModalProps>`
height,
draggable,
hideFooter,
- }) => css`
- ${responsive &&
- css`
- max-width: ${maxWidth ?? '900px'};
- padding-left: ${theme.sizeUnit * 3}px;
- padding-right: ${theme.sizeUnit * 3}px;
- padding-bottom: 0;
- top: 0;
- `}
-
- .ant-modal-content {
- background-color: ${theme.colorBgContainer};
- display: flex;
- flex-direction: column;
- max-height: calc(100vh - ${theme.sizeUnit * 8}px);
- margin-bottom: ${theme.sizeUnit * 4}px;
- margin-top: ${theme.sizeUnit * 4}px;
- padding: 0;
- }
+ }) => {
+ const closeButtonWidth = theme.sizeUnit * 14;
+
+ return css`
+ ${responsive &&
+ css`
+ max-width: ${maxWidth ?? '900px'};
+ padding-left: ${theme.sizeUnit * 3}px;
+ padding-right: ${theme.sizeUnit * 3}px;
+ padding-bottom: 0;
+ top: 0;
+ `}
+
+ .ant-modal-content {
+ background-color: ${theme.colorBgContainer};
+ display: flex;
+ flex-direction: column;
+ max-height: calc(100vh - ${theme.sizeUnit * 8}px);
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ margin-top: ${theme.sizeUnit * 4}px;
+ padding: 0;
+ }
+
+ .ant-modal-header {
+ flex: 0 0 auto;
+ border-radius: ${theme.borderRadius}px ${theme.borderRadius}px 0 0;
+ padding: ${theme.sizeUnit * 4}px ${closeButtonWidth}px
+ ${theme.sizeUnit * 4}px ${theme.sizeUnit * 4}px;
- .ant-modal-header {
- flex: 0 0 auto;
- border-radius: ${theme.borderRadius}px ${theme.borderRadius}px 0 0;
- padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 4}px;
+ .ant-modal-title {
+ font-weight: ${theme.fontWeightStrong};
+ }
- .ant-modal-title {
- font-weight: ${theme.fontWeightStrong};
+ .ant-modal-title h4 {
+ display: flex;
+ margin: 0;
+ align-items: center;
+ }
}
- .ant-modal-title h4 {
+ .ant-modal-close {
+ width: ${closeButtonWidth}px;
+ height: ${theme.sizeUnit * 14}px;
+ padding: ${theme.sizeUnit * 6}px ${theme.sizeUnit * 4}px
+ ${theme.sizeUnit * 4}px;
+ top: 0;
+ right: 0;
display: flex;
- margin: 0;
- align-items: center;
+ justify-content: center;
}
- }
-
- .ant-modal-close {
- width: ${theme.sizeUnit * 14}px;
- height: ${theme.sizeUnit * 14}px;
- padding: ${theme.sizeUnit * 6}px ${theme.sizeUnit * 4}px
- ${theme.sizeUnit * 4}px;
- top: 0;
- right: 0;
- display: flex;
- justify-content: center;
- }
-
- .ant-modal-close:hover {
- background: transparent;
- }
- .ant-modal-close-x {
- display: flex;
- align-items: center;
- [data-test='close-modal-btn'] {
- justify-content: center;
+ .ant-modal-close:hover {
+ background: transparent;
}
- .close {
- flex: 1 1 auto;
- margin-bottom: ${theme.sizeUnit}px;
- color: ${theme.colorPrimaryText};
- font-weight: ${theme.fontWeightLight};
+
+ .ant-modal-close-x {
+ display: flex;
+ align-items: center;
+ [data-test='close-modal-btn'] {
+ justify-content: center;
+ }
+ .close {
+ flex: 1 1 auto;
+ margin-bottom: ${theme.sizeUnit}px;
+ color: ${theme.colorPrimaryText};
+ font-weight: ${theme.fontWeightLight};
+ }
}
- }
- .ant-modal-body {
- flex: 0 1 auto;
- padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 6}px;
+ .ant-modal-body {
+ flex: 0 1 auto;
+ padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 6}px;
- overflow: auto;
- ${!resizable && height && `height: ${height};`}
- }
+ overflow: auto;
+ ${!resizable && height && `height: ${height};`}
+ }
- .ant-modal-footer {
- flex: 0 0 1;
- border-top: ${theme.sizeUnit / 4}px solid ${theme.colorSplit};
- padding: ${theme.sizeUnit * 4}px;
- margin-top: 0;
+ .ant-modal-footer {
+ flex: 0 0 1;
+ border-top: ${theme.sizeUnit / 4}px solid ${theme.colorSplit};
+ padding: ${theme.sizeUnit * 4}px;
+ margin-top: 0;
- .btn {
- font-size: 12px;
- }
+ .btn {
+ font-size: 12px;
+ }
- .btn + .btn {
- margin-left: ${theme.sizeUnit * 2}px;
+ .btn + .btn {
+ margin-left: ${theme.sizeUnit * 2}px;
+ }
}
- }
- &.no-content-padding .ant-modal-body {
- padding: 0;
- }
-
- ${draggable &&
- css`
- .ant-modal-header {
+ &.no-content-padding .ant-modal-body {
padding: 0;
-
- .draggable-trigger {
- cursor: move;
- padding: ${theme.sizeUnit * 4}px;
- width: 100%;
- }
}
- `}
- ${resizable &&
- css`
- .resizable {
- pointer-events: all;
+ ${draggable &&
+ css`
+ .ant-modal-header {
+ padding: 0;
- .resizable-wrapper {
- height: 100%;
+ .draggable-trigger {
+ cursor: move;
+ padding: ${theme.sizeUnit * 4}px;
+ width: 100%;
Review Comment:
**Suggestion:** When `draggable` is enabled the `.ant-modal-header` padding
is removed and the `.draggable-trigger` is set to width:100%, but no right
padding is preserved for the close button; this allows long titles to extend
under the close button. Add a right padding equal to the close button width (or
otherwise reserve space) to `.draggable-trigger`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
padding-right: ${closeButtonWidth}px;
width: 100%;
box-sizing: border-box;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
When draggable mode removes header padding, long titles can extend beneath
the close control. Reserving right padding equal to the close button width (and
using box-sizing) prevents overlap. The suggested change uses the
already-declared `closeButtonWidth` so it's safe and appropriate in this scope.
</details>
<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/Modal/Modal.tsx
**Line:** 162:162
**Comment:**
*Logic Error: When `draggable` is enabled the `.ant-modal-header`
padding is removed and the `.draggable-trigger` is set to width:100%, but no
right padding is preserved for the close button; this allows long titles to
extend under the close button. Add a right padding equal to the close button
width (or otherwise reserve space) to `.draggable-trigger`.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:
##########
@@ -55,132 +55,137 @@ export const StyledModal =
styled(BaseModal)<StyledModalProps>`
height,
draggable,
hideFooter,
- }) => css`
- ${responsive &&
- css`
- max-width: ${maxWidth ?? '900px'};
- padding-left: ${theme.sizeUnit * 3}px;
- padding-right: ${theme.sizeUnit * 3}px;
- padding-bottom: 0;
- top: 0;
- `}
-
- .ant-modal-content {
- background-color: ${theme.colorBgContainer};
- display: flex;
- flex-direction: column;
- max-height: calc(100vh - ${theme.sizeUnit * 8}px);
- margin-bottom: ${theme.sizeUnit * 4}px;
- margin-top: ${theme.sizeUnit * 4}px;
- padding: 0;
- }
+ }) => {
+ const closeButtonWidth = theme.sizeUnit * 14;
+
+ return css`
+ ${responsive &&
+ css`
+ max-width: ${maxWidth ?? '900px'};
+ padding-left: ${theme.sizeUnit * 3}px;
+ padding-right: ${theme.sizeUnit * 3}px;
+ padding-bottom: 0;
+ top: 0;
+ `}
+
+ .ant-modal-content {
+ background-color: ${theme.colorBgContainer};
+ display: flex;
+ flex-direction: column;
+ max-height: calc(100vh - ${theme.sizeUnit * 8}px);
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ margin-top: ${theme.sizeUnit * 4}px;
+ padding: 0;
+ }
+
+ .ant-modal-header {
+ flex: 0 0 auto;
+ border-radius: ${theme.borderRadius}px ${theme.borderRadius}px 0 0;
+ padding: ${theme.sizeUnit * 4}px ${closeButtonWidth}px
+ ${theme.sizeUnit * 4}px ${theme.sizeUnit * 4}px;
- .ant-modal-header {
- flex: 0 0 auto;
- border-radius: ${theme.borderRadius}px ${theme.borderRadius}px 0 0;
- padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 4}px;
+ .ant-modal-title {
+ font-weight: ${theme.fontWeightStrong};
+ }
- .ant-modal-title {
- font-weight: ${theme.fontWeightStrong};
+ .ant-modal-title h4 {
+ display: flex;
+ margin: 0;
+ align-items: center;
+ }
}
- .ant-modal-title h4 {
+ .ant-modal-close {
+ width: ${closeButtonWidth}px;
+ height: ${theme.sizeUnit * 14}px;
+ padding: ${theme.sizeUnit * 6}px ${theme.sizeUnit * 4}px
+ ${theme.sizeUnit * 4}px;
+ top: 0;
+ right: 0;
display: flex;
- margin: 0;
- align-items: center;
+ justify-content: center;
}
- }
-
- .ant-modal-close {
- width: ${theme.sizeUnit * 14}px;
- height: ${theme.sizeUnit * 14}px;
- padding: ${theme.sizeUnit * 6}px ${theme.sizeUnit * 4}px
- ${theme.sizeUnit * 4}px;
- top: 0;
- right: 0;
- display: flex;
- justify-content: center;
- }
-
- .ant-modal-close:hover {
- background: transparent;
- }
- .ant-modal-close-x {
- display: flex;
- align-items: center;
- [data-test='close-modal-btn'] {
- justify-content: center;
+ .ant-modal-close:hover {
+ background: transparent;
}
- .close {
- flex: 1 1 auto;
- margin-bottom: ${theme.sizeUnit}px;
- color: ${theme.colorPrimaryText};
- font-weight: ${theme.fontWeightLight};
+
+ .ant-modal-close-x {
+ display: flex;
+ align-items: center;
+ [data-test='close-modal-btn'] {
+ justify-content: center;
+ }
+ .close {
+ flex: 1 1 auto;
Review Comment:
**Suggestion:** The close button `.close` is styled with `flex: 1 1 auto`,
which allows the close button to grow and consume available header space; that
can push or shrink the title and reintroduce overlap or layout issues. Make the
close icon non-growing so it doesn't take space intended for the title.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
flex: 0 0 auto;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current rule `flex: 1 1 auto` can cause the close element to grow and
eat header space, which is likely to cause title overlap. Changing it to a
non-growing flex (`0 0 auto`) makes the layout more robust without any
functional regressions. This is a targeted layout fix tied to the diff.
</details>
<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/Modal/Modal.tsx
**Line:** 120:120
**Comment:**
*Possible Bug: The close button `.close` is styled with `flex: 1 1
auto`, which allows the close button to grow and consume available header
space; that can push or shrink the title and reintroduce overlap or layout
issues. Make the close icon non-growing so it doesn't take space intended for
the title.
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.
```
</details>
--
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]
