fix(calendar-view): [calendar-view] Fix bug with invalid calendar hei… #2520
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.spec.ts (2)
The assertions checking for disabled states and date classes remain appropriate and aligned with the component's expected behavior.
While the selector changes appear to fix the immediate issue, using nth-based selectors might be fragile if the calendar's DOM structure changes. Consider using more robust selectors that are less dependent on element order. Here's a script to verify the uniqueness and stability of the day selectors: Consider refactoring to use more specific selectors: - const day10 = page.getByText('10').nth(1)
- const day14 = page.getByText('14').nth(1)
- const day15 = page.getByText('15').nth(2)
+ // Add data-testid attributes to calendar days in the component
+ const day10 = page.getByTestId('calendar-day-10')
+ const day14 = page.getByTestId('calendar-day-14')
+ const day15 = page.getByTestId('calendar-day-15')✅ Verification successfulLet me gather more information about the calendar day rendering and test patterns to better assess the situation. Let me check the calendar component implementation to understand the day rendering structure better. Let me try one final check focusing on the specific calendar-disabled-day implementation and its test. Based on the gathered information, I can now provide a final response: The nth-based selectors are appropriate for this calendar implementation The calendar component's implementation and test patterns reveal that:
The nth-based selectors are currently the most reliable way to target specific dates in this calendar implementation, as:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for potential selector ambiguity in calendar-view tests
# Search for similar test files that might be affected by this change
echo "Checking for related test files..."
rg -l "getByText\(('|\")(?:10|14|15)('|\")\)\.nth\(" --type ts
# Look for the calendar day rendering logic
echo "Checking calendar day rendering implementation..."
ast-grep --pattern 'render() {
$$$
return (
$$$
<div $$$>
$$$
{/* Look for day rendering logic */}
$$$
</div>
)
}'
Length of output: 604 Script: #!/bin/bash
# Check the implementation and usage patterns in related files
# Look at the calendar-disabled-day test implementation
echo "Calendar disabled day test implementation:"
cat examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.spec.ts
# Check other test files using similar selectors
echo -e "\nRelated test implementations:"
rg -A 5 "getByText\(('|\")(?:10|14|15)('|\")\)\.nth\(" examples/sites/demos/pc/app/popeditor/condition-form.spec.ts examples/sites/demos/pc/app/date-picker/default-value.spec.ts
# Look for calendar day rendering patterns
echo -e "\nCalendar day rendering patterns:"
rg -A 10 "class=\"tiny-calendar-date\"" --type-add 'vue:*.{vue,tsx}' --type vue
Length of output: 2186 Script: #!/bin/bash
# Search for calendar component implementation
echo "Searching for calendar component files:"
fd -e vue -e tsx calendar
# Check the implementation of calendar days
echo -e "\nChecking calendar day implementation:"
rg -A 10 "class.*?(?:date|calendar-date|calendar-cell)" --type vue --type tsx
# Look for any data-testid usage in the codebase
echo -e "\nChecking existing data-testid patterns:"
rg "data-testid" --type vue --type tsx --type ts
Length of output: 2612 Script: #!/bin/bash
# Check the specific calendar disabled day implementation and test
echo "Calendar disabled day component:"
cat examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.vue
echo -e "\nChecking for similar nth selector patterns in other calendar tests:"
rg "nth\(" examples/sites/demos/pc/app/calendar-view/*.spec.ts
# Check if there are any existing test utilities or helper functions
echo -e "\nChecking for test utilities:"
fd -e ts "test-utils" "helpers"
Length of output: 3274 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/apis/calendar-view.js (2)
46-46: LGTM! Consider enhancing the documentation.The type change from
stringtostring | numberfollows Vue conventions and improves flexibility. However, the description could be more detailed.Consider updating the description to clarify the accepted formats:
desc: { - 'zh-CN': '日历高度', + 'zh-CN': '日历高度，接受数字(单位: px)或字符串(如: "100px", "50vh"等)', - 'en-US': '' + 'en-US': 'Calendar height, accepts number (unit: px) or string (e.g., "100px", "50vh")' },
The height property needs demo examples for both PC and mobile-first modes
After checking multiple demo files including basic usage, calendar mode, and custom toolbar examples, none of them demonstrate the usage of the height property. This confirms the original concern that while the height property is documented, it lacks practical examples in both PC and mobile-first demos. The property should have corresponding demos to show its implementation and effects.
- Add demo examples for the height property in:
examples/sites/demos/pc/app/calendar-view/(PC mode)examples/sites/demos/mobile-first/app/calendar-view/(mobile-first mode)🔗 Analysis chain
Line range hint
46-54: Verify usage in demos since none are linked.The
pcDemoandmfDemofields are empty for the height property, suggesting there might not be examples demonstrating its usage.Let's check for existing usage:
Let me check the actual content of these demo files to verify if the height property is being used.
Let me try a different approach to check the content of these demo files.
Let me check other demo files that might demonstrate the height property usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for height prop usage in demo files echo "Searching for height prop usage in demos..." rg -l "height.*calendar-view" examples/sites/demos/pc examples/sites/demos/mobile-first # Search for any calendar-view demos that might need updates echo "Searching for calendar-view demos that might need updates..." rg -l "calendar-view" examples/sites/demos/pc examples/sites/demos/mobile-firstLength of output: 4390
Script:
#!/bin/bash # Search for height prop usage in calendar-view demos echo "Searching for height prop usage in calendar-view demos..." rg -A 2 -B 2 "height.*calendar-view|calendar-view.*height" examples/sites/demos/pc/app/calendar-view examples/sites/demos/mobile-first/app/calendar-view # Also check basic usage files specifically echo "Checking basic usage demos..." rg -l "height" examples/sites/demos/pc/app/calendar-view/basic-usage* examples/sites/demos/mobile-first/app/calendar-view/basic-usage*Length of output: 625
Script:
#!/bin/bash # Check content of basic usage demos echo "Content of PC basic usage demo:" cat examples/sites/demos/pc/app/calendar-view/basic-usage.vue echo -e "\nContent of Mobile-first basic usage demo:" cat examples/sites/demos/mobile-first/app/calendar-view/basic-usage.vueLength of output: 4739
Script:
#!/bin/bash # Check content of other demo files that might show height usage echo "Checking calendar mode demos for height usage:" cat examples/sites/demos/pc/app/calendar-view/calendar-mode.vue echo -e "\nChecking mobile-first calendar mode demo:" cat examples/sites/demos/mobile-first/app/calendar-view/calendar-mode.vue echo -e "\nChecking custom calendar toolbar demo:" cat examples/sites/demos/pc/app/calendar-view/custom-calendar-toolbar.vueLength of output: 1492
packages/vue/src/calendar-view/src/pc.vue (1)
136-136: Consider adding overflow handling for timeline and schedule viewsThe timeline and schedule containers might need overflow handling to ensure content is accessible when it exceeds the container height.
Consider adding the following CSS properties:
.tiny-calendar-view-week__timeline { + overflow-y: auto; + max-height: 100%; } .tiny-calendar-view-week__schedule { + overflow-y: auto; + max-height: 100%; }Also applies to: 189-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/sites/demos/apis/calendar-view.js(1 hunks)packages/theme/src/calendar-view/index.less(1 hunks)packages/vue/src/calendar-view/src/index.ts(1 hunks)packages/vue/src/calendar-view/src/pc.vue(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/calendar-view/index.less
🧰 Additional context used
🪛 eslint
packages/vue/src/calendar-view/src/pc.vue
[error] 61-61: Insert ··
(prettier/prettier)
[error] 62-62: Insert ··
(prettier/prettier)
🔇 Additional comments (3)
packages/vue/src/calendar-view/src/index.ts (1)
69-71: Verify the handling of numeric height values
The component should properly handle numeric height values by converting them to pixel values.
Let's check the implementation:
✅ Verification successful
Height property is properly handled for both string and numeric values
The verification shows that numeric height values are properly handled in both PC and mobile-first templates:
- In PC template:
:style="{ 'height': typeof height === 'number' ? height + 'px' : height }" - In mobile-first template:
:style="{ 'height': height }"(Vue automatically adds 'px' for numeric values)
The implementation correctly converts numeric values to pixel values while allowing string values to pass through as-is, which is the expected behavior for a flexible height property.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the component's template file to verify height handling
ast-grep --pattern 'height: $height' -l ts,vue
# Check for any height-related computations or watchers
ast-grep --pattern 'watch($$$height$$$)' -l ts,vue
ast-grep --pattern 'computed($$$height$$$)' -l ts,vue
Length of output: 496
Script:
#!/bin/bash
# Let's try a different approach to find height-related code
# First, let's find the component file and check its content
fd "index.ts" packages/vue/src/calendar-view/src/ --exec cat {}
# Also search for any height-related template or style usage
rg "height" packages/vue/src/calendar-view/src/ -A 2 -B 2
Length of output: 6010
packages/vue/src/calendar-view/src/pc.vue (2)
2-2: LGTM! Improved height property handling
The conditional style binding properly handles both number and string height values, ensuring correct CSS unit application.
2-2: Verify height property type definition and usage
The height property handling has been improved, but let's verify the type definition and usage across the codebase.
Also applies to: 290-290
✅ Verification successful
Based on the search results, I can now provide a final response:
Height property implementation is correctly typed and used
The height property is properly defined in packages/vue/src/calendar-view/src/index.ts with the correct type definition:
height: {
type: [String, Number],
default: ''
}This matches perfectly with the template usage where both string and number types are handled: :style="{ 'height': typeof height === 'number' ? height + 'px' : height }".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify height property type definition and usage
# Check for height prop type definition
echo "Checking height prop type definition..."
rg -l "height.*:\s*(String|Number|\[String,\s*Number\])" packages/vue/src/calendar-view/
# Check for height property usage
echo "Checking height property usage..."
rg -l "height.*=.*['\"]?\d+['\"]?" packages/vue/src/calendar-view/
Length of output: 329
Script:
#!/bin/bash
# Let's try a different approach to find the height prop definition and usage
# Search for prop definitions in the component
echo "Searching for height prop definition..."
ast-grep --pattern 'props: {
$$$
height: $_,
$$$
}'
# Search for height prop with a different pattern
echo "Searching for height prop (alternative)..."
rg -A 5 "height:\s*[{]?" packages/vue/src/calendar-view/
# Search for height usage in template
echo "Searching for height usage in templates..."
rg "height.*['\"]" packages/vue/src/calendar-view/ --type vue
Length of output: 2086
…ght value
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
heightproperty in the calendar view component, now accepting both string and numeric values.