feat(modal): adapt to Modal Components#4129
Closed
James-9696 wants to merge 5 commits intoopentiny:devfrom
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
WalkthroughRemoves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/theme/src/modal/responsive.less`:
- Around line 50-65: At the 600px boundary both media queries match causing the
second to override the first; make the breakpoint exclusive by changing one of
the media queries so only one matches at 600px (for example update the first to
`@media` (min-width: 601px) and (max-width: 1280px) or update the second to `@media`
screen and (max-width: 599px)), leaving the .modal-responsive calls and their
params unchanged so the 600px width has a single owner.
- Around line 58-63: The modal uses a hard-coded `@box-width`: 320px inside
.modal-responsive which, combined with horizontal margins, causes overflow under
352px; change the `@box-width` argument in the .modal-responsive invocation to be
responsive (for example use a max-width value or compute it from the
viewport/margins such as max-width: 100% or calc(100% - 2 *
`@modal-adapt-space-part`) or set `@box-width`: auto) so the modal shrinks on very
narrow screens and doesn’t overflow when the viewport is below 352px.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d07df58-b00e-40d5-ac49-2888107e6638
📒 Files selected for processing (3)
packages/theme/src/adaptation-index.lesspackages/theme/src/modal/responsive.lesspackages/theme/src/modal/vars.less
James-9696
changed the title
James-9696
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/theme/src/modal/responsive.less (1)
18-18: Consider makingtopresponsive.The
top: 100pxvalue is hardcoded while other spacing values use the responsive CSS custom properties. For consistency with the small-screen adaptation goal, consider using a responsive variable or at least a smaller value for the narrow-screen media query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/theme/src/modal/responsive.less` at line 18, The hardcoded "top: 100px" in packages/theme/src/modal/responsive.less should be made responsive: replace the literal with your responsive CSS custom property (e.g., top: var(--modal-top, 100px)) and ensure the narrow-screen media query overrides that variable (or sets a smaller value like var(--modal-top-sm, 64px)). Update the declaration where "top: 100px" appears and add or adjust the corresponding CSS custom properties used elsewhere for spacing so the modal spacing adapts on small screens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/theme/src/modal/responsive.less`:
- Around line 17-21: The .@{modal-prefix-cls}__box rule currently uses
transform: translateX(-5%) without an explicit horizontal anchor, causing
inconsistent placement; change the horizontal centering to use left: 50%
together with transform: translateX(-50%) in the .@{modal-prefix-cls}__box rule
(or, if the -5% offset was intentional, add a comment explaining the rationale
and keep the existing transform), and ensure no conflicting left/right
properties remain that would override centering.
---
Nitpick comments:
In `@packages/theme/src/modal/responsive.less`:
- Line 18: The hardcoded "top: 100px" in
packages/theme/src/modal/responsive.less should be made responsive: replace the
literal with your responsive CSS custom property (e.g., top: var(--modal-top,
100px)) and ensure the narrow-screen media query overrides that variable (or
sets a smaller value like var(--modal-top-sm, 64px)). Update the declaration
where "top: 100px" appears and add or adjust the corresponding CSS custom
properties used elsewhere for spacing so the modal spacing adapts on small
screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 560fa0bf-e734-4d2e-84e4-63821ae818f1
📒 Files selected for processing (2)
packages/theme/src/modal/responsive.lesspackages/theme/src/modal/vars.less
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/modal/vars.less
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/theme/src/modal/responsive.less (2)
19-20:⚠️ Potential issue | 🟡 MinorCentering still appears offset left.
With
left: 50%on Line 19,translateX(-55%)on Line 20 shifts the modal an extra 5% left. If true centering is intended, use-50%(or document why the offset is intentional).💡 Suggested fix
- transform: translateX(-55%); + transform: translateX(-50%);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/theme/src/modal/responsive.less` around lines 19 - 20, The modal centering is offset because the rule using transform: translateX(-55%) shifts it 5% further left relative to left: 50%; update the transform in the responsive modal CSS (the rule containing left: 50% and transform: translateX(...)) to use translateX(-50%) for true centering, or if the 5% offset is intentional add a comment explaining why and keep translateX(-55%).
63-64:⚠️ Potential issue | 🟠 MajorMobile width can still overflow on very narrow screens.
At Line 64, fixed
320pxwidth plus horizontal margins can exceed viewport width on smaller devices, which conflicts with the small-screen adaptation goal.💡 Suggested fix
`@media` screen and (max-width: 599px) { .modal-responsive( `@header-padding`: `@modal-responsive-space-part` `@modal-responsive-space-part`; `@body-padding`: 0 `@modal-responsive-space-part`; `@box-margin`: 0 `@modal-responsive-space-part` 50px; - `@box-width`: 320px; + `@box-width`: min(320px, calc(100vw - (2 * `@modal-responsive-space-part`))); ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/theme/src/modal/responsive.less` around lines 63 - 64, The fixed `@box-width`: 320px can cause horizontal overflow on very narrow viewports; update the responsive rule to use a fluid constraint (e.g. replace the fixed `@box-width` with a responsive expression such as using CSS min()/max-width semantics) so the modal never exceeds the viewport width — change the `@box-width` variable to a responsive value (for example use min(320px, 100%) or set width: 100% with max-width: 320px where the modal uses `@box-width`) and ensure the modal layout that uses `@box-margin` and `@box-width` respects those max-width constraints to prevent overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/theme/src/modal/responsive.less`:
- Around line 19-20: The modal centering is offset because the rule using
transform: translateX(-55%) shifts it 5% further left relative to left: 50%;
update the transform in the responsive modal CSS (the rule containing left: 50%
and transform: translateX(...)) to use translateX(-50%) for true centering, or
if the 5% offset is intentional add a comment explaining why and keep
translateX(-55%).
- Around line 63-64: The fixed `@box-width`: 320px can cause horizontal overflow
on very narrow viewports; update the responsive rule to use a fluid constraint
(e.g. replace the fixed `@box-width` with a responsive expression such as using
CSS min()/max-width semantics) so the modal never exceeds the viewport width —
change the `@box-width` variable to a responsive value (for example use min(320px,
100%) or set width: 100% with max-width: 320px where the modal uses `@box-width`)
and ensure the modal layout that uses `@box-margin` and `@box-width` respects those
max-width constraints to prevent overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbb4ef5c-f4e5-4aba-8b65-cf6b1c3f7559
📒 Files selected for processing (1)
packages/theme/src/modal/responsive.less
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/theme/src/modal/responsive.less (2)
17-22:⚠️ Potential issue | 🟠 MajorUse true centering for the modal box.
At Line 20,
translateX(-55%)over-shifts the dialog left relative toleft: 50%, so alert/confirm modals are no longer centered.💡 Suggested fix
.@{modal-prefix-cls}__box { top: 100px; left: 50% !important; - transform: translateX(-55%); + transform: translateX(-50%); margin: `@box-margin`; width: `@box-width`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/theme/src/modal/responsive.less` around lines 17 - 22, The modal box rule .@{modal-prefix-cls}__box currently uses left: 50% with transform: translateX(-55%) causing mis-centering; change the transform to translateX(-50%) (or use transform: translate(-50%, 0)) so the dialog is truly centered horizontally when combined with left: 50%; update the CSS rule in responsive.less for .@{modal-prefix-cls}__box accordingly.
59-65:⚠️ Potential issue | 🟠 MajorAvoid fixed 320px width on ultra-narrow screens.
At Line 64, fixed
320pxwidth combined with horizontal margins (Line 63) can exceed viewport width on devices below ~352px, causing horizontal overflow.💡 Suggested fix
`@media` screen and (max-width: 599px) { .modal-responsive( `@header-padding`: `@modal-responsive-space-part` `@modal-responsive-space-part`; `@body-padding`: 0 `@modal-responsive-space-part`; `@box-margin`: 0 `@modal-responsive-space-part` 50px; - `@box-width`: 320px; + `@box-width`: min(320px, calc(100vw - (2 * `@modal-responsive-space-part`))); ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/theme/src/modal/responsive.less` around lines 59 - 65, The modal uses a fixed `@box-width`: 320px which together with `@box-margin` can exceed very narrow viewports; update the .modal-responsive parameters so `@box-width` is responsive (e.g. set it to a min/max expression or percentage that accounts for the left/right margins) instead of a fixed 320px and ensure the computed width uses the horizontal spacing from `@box-margin/`@modal-responsive-space-part so the modal never causes horizontal overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/theme/src/modal/responsive.less`:
- Around line 17-22: The modal box rule .@{modal-prefix-cls}__box currently uses
left: 50% with transform: translateX(-55%) causing mis-centering; change the
transform to translateX(-50%) (or use transform: translate(-50%, 0)) so the
dialog is truly centered horizontally when combined with left: 50%; update the
CSS rule in responsive.less for .@{modal-prefix-cls}__box accordingly.
- Around line 59-65: The modal uses a fixed `@box-width`: 320px which together
with `@box-margin` can exceed very narrow viewports; update the .modal-responsive
parameters so `@box-width` is responsive (e.g. set it to a min/max expression or
percentage that accounts for the left/right margins) instead of a fixed 320px
and ensure the computed width uses the horizontal spacing from
`@box-margin/`@modal-responsive-space-part so the modal never causes horizontal
overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1bd827d1-3544-4134-a385-ae1ba2780f18
📒 Files selected for processing (2)
packages/theme/src/modal/responsive.lesspackages/theme/src/modal/vars.less
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/modal/vars.less
James-9696
changed the title
github-actions
Bot
added
enhancement
zzcr
previously approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
适配小屏幕显示
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit