fix(modal): 修复 modal 组件在 alert, confirm 的弹窗场景下 top 属性不生效的问题 #3919
Conversation
WalkthroughGeneralizes the modal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
gausszhou
force-pushed
the
bugfix/modal_top
branch
from
1e246e8 to
9954caa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderless/src/modal/index.ts (1)
57-81: Fix the return type signature to includetop.The function return type on line 59 is missing the
topproperty, but line 80 returns an object containingtop. This creates a TypeScript type mismatch.🔎 Apply this diff to fix the return type:
export const computedBoxStyle = ({ props, isMobileFirstMode }: Pick<IModalRenderlessParams, 'props' | 'isMobileFirstMode'>) => - (): { width?: string | number; height?: string | number } => { + (): { top?: string; width?: string | number; height?: string | number } => { if (isMobileFirstMode) { return {} }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/sites/demos/apis/modal.js(1 hunks)examples/sites/demos/pc/app/modal/message-top.vue(2 hunks)examples/sites/demos/pc/app/modal/webdoc/modal.js(1 hunks)packages/renderless/src/modal/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (4)
packages/renderless/src/modal/index.ts (1)
64-70: LGTM! The top positioning logic is correctly implemented.The logic properly computes the
topstyle for 'alert' and 'confirm' modal types, handling both numeric and string values, which successfully extends the feature beyond 'message' type modals.examples/sites/demos/apis/modal.js (1)
382-382: Good improvement adding explicit unit to default value.Changing the default from '80' to '80px' improves clarity and consistency.
examples/sites/demos/pc/app/modal/webdoc/modal.js (1)
304-308: LGTM! Documentation correctly reflects the broader applicability.The Chinese label and description have been properly updated from message-specific ("消息距离顶部位置") to general modal positioning ("模态框距离顶部位置"), aligning with the expanded feature scope.
examples/sites/demos/pc/app/modal/message-top.vue (1)
19-39: Excellent demo coverage across modal types and value formats.The three handlers effectively demonstrate the
topproperty working across all modal types (alert, confirm, message) with both numeric pixel values and CSS units (vh), providing comprehensive validation of the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/renderless/src/modal/index.ts (1)
57-81: LGTM! Core logic correctly implementstoppositioning for alert/confirm modals.The implementation properly:
- Checks for 'alert' or 'confirm' types
- Formats numeric values with 'px' suffix
- Preserves string values like '50vh' as-is
- Updates the return type signature appropriately
The logic aligns well with the existing
widthandheighthandling patterns.Optional: Consider only including `top` in the return object when it's actually set
Currently, an empty string is returned for
topwhen the type is not 'alert' or 'confirm'. While this matches the existing pattern forwidthandheight, you could make the return more explicit:- return { top, width, height } + const style: { top?: string; width?: string | number; height?: string | number } = {} + if (top) style.top = top + if (width) style.width = width + if (height) style.height = height + return styleThis ensures only defined values are included in the returned style object.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/sites/demos/apis/modal.js(1 hunks)examples/sites/demos/pc/app/modal/message-top.vue(2 hunks)examples/sites/demos/pc/app/modal/webdoc/modal.js(1 hunks)packages/renderless/src/modal/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (3)
examples/sites/demos/pc/app/modal/webdoc/modal.js (1)
302-312: Documentation update correctly reflects the expanded scope of thetopproperty.The Chinese name and description changes appropriately generalize from message-specific ("消息距离顶部位置") to modal-wide ("模态框距离顶部位置") positioning, aligning with the code changes that extend
topsupport to alert and confirm modal types.examples/sites/demos/apis/modal.js (1)
384-385: Description updates correctly reflect modal-wide positioning.The updated descriptions appropriately generalize the
topproperty from message-specific to modal-wide positioning, consistent with the code changes.examples/sites/demos/pc/app/modal/message-top.vue (1)
1-42: Excellent demo implementation showcasing the expandedtopproperty functionality.The three handlers effectively demonstrate:
- Alert modal with
top: 300(numeric, new functionality)- Confirm modal with
top: '50vh'(string with viewport units, new functionality)- Message modal with
top: 500(existing functionality preserved)This comprehensively covers both the new alert/confirm support and various value formats (number, string with units), making it clear to developers how the
topproperty now works across all modal types.
| name: 'top', | ||
| type: 'number | string', | ||
| defaultValue: '80', | ||
| defaultValue: '80px', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the modal.js file and examine the top property default value
find . -name "modal.js" -type f 2>/dev/null | head -20Repository: opentiny/tiny-vue
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Read the modal.js file around line 382
sed -n '375,390p' ./examples/sites/demos/apis/modal.js
# Also search for where the top property is defined in Modal component
grep -n "top.*default" ./examples/sites/demos/apis/modal.js | head -20Repository: opentiny/tiny-vue
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Search for the Modal component's top prop definition in the source code
find . -path ./node_modules -prune -o -name "index.ts" -type f -print | xargs grep -l "computedBoxStyle" 2>/dev/null | head -5
# Also search for Modal component files
find . -path ./node_modules -prune -o -name "*modal*" -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.vue" \) -print | grep -E "src|packages" | head -20Repository: opentiny/tiny-vue
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Read the computedBoxStyle function in modal index.ts
sed -n '60,90p' ./packages/renderless/src/modal/index.ts
# Also search for the complete computedBoxStyle function
grep -n "computedBoxStyle" ./packages/renderless/src/modal/index.ts
# Get more context around that area
sed -n '1,120p' ./packages/renderless/src/modal/index.ts | tail -80Repository: opentiny/tiny-vue
Length of output: 2936
🏁 Script executed:
#!/bin/bash
# Search for the Modal component props definition to find the actual default value for 'top'
find . -path ./node_modules -prune -o -path ./packages/renderless -prune -o -name "*modal*" -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.vue" \) -print | xargs grep -l "top.*default" 2>/dev/null | head -10
# Also check the Modal type definitions
grep -n "top" ./packages/renderless/types/modal.type.ts | head -20Repository: opentiny/tiny-vue
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Find the actual Modal component implementation with prop defaults
find ./packages -name "*modal*" \( -name "*.ts" -o -name "*.tsx" -o -name "*.vue" \) -type f | xargs grep -l "props.*top" 2>/dev/null | head -10
# Also check Vue component definition files
find ./packages/vue/src/modal -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.vue" \) | xargs cat 2>/dev/null | head -200Repository: opentiny/tiny-vue
Length of output: 5062
🏁 Script executed:
#!/bin/bash
# Look for the Modal component's props definition more specifically
find ./packages/vue/src/modal -name "*.vue" -o -name "*.ts" -o -name "*.tsx" | xargs grep -l "top" 2>/dev/null
# Check the main Modal component file
ls -la ./packages/vue/src/modal/
# Read the main Modal component
cat ./packages/vue/src/modal/src/modal.tsx 2>/dev/null | head -150Repository: opentiny/tiny-vue
Length of output: 586
🏁 Script executed:
#!/bin/bash
# Read the Modal component index.ts to find props definition
cat ./packages/vue/src/modal/src/index.ts
# Also check the pc.vue component
head -100 ./packages/vue/src/modal/src/pc.vueRepository: opentiny/tiny-vue
Length of output: 5472
🏁 Script executed:
#!/bin/bash
# Verify the exact prop default for top and check how it's rendered
grep -A 1 "top:" ./packages/vue/src/modal/src/index.ts
# Also check if there are any comments about this in the computedBoxStyle
sed -n '55,90p' ./packages/renderless/src/modal/index.tsRepository: opentiny/tiny-vue
Length of output: 1112
Correct the documentation for the top property default value.
The top property default is a number 80 (not the string '80px'). The computedBoxStyle function in packages/renderless/src/modal/index.ts automatically converts numeric values to strings with the 'px' suffix for alert and confirm modal types. Update the defaultValue in the documentation from '80px' to 80 and add a note explaining that the suffix is automatically appended.
🤖 Prompt for AI Agents
In examples/sites/demos/apis/modal.js around line 382, update the documented
default for the `top` property from the string '80px' to the number 80 and add a
brief note that the renderless computedBoxStyle function will automatically
append the 'px' suffix for alert/confirm modal types; change the defaultValue to
80 and include a one-line comment or note indicating the px suffix is appended
automatically.
|
你好，E2E测试没过，看下哈 |
Hello, I haven’t passed the E2E test. Let’s take a look. |
3 participants
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?
modal 组件在 alert, confirm 的弹窗场景下 top 属性不生效
Issue Number: N/A
What is the new behavior?
modal 组件在 alert, confirm 的弹窗场景下 top 属性生效
Does this PR introduce a breaking change?
Other information
此改动会影响初始值从 15vh 变为 80px
Summary by CodeRabbit
New Features
toppositioning now applies to alert and confirm modal types and is supported with unit-aware values.Bug Fixes
topvalue formatted to include units (e.g., "80px") to ensure consistent positioning.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.