Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(calendar-view): [calendar-view] Fix bug with invalid calendar hei… #2520

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Youyou-smiles
Copy link
Collaborator

@Youyou-smiles Youyou-smiles commented Nov 14, 2024

…ght value

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

  • New Features
    • Enhanced flexibility for the height property in the calendar view component, now accepting both string and numeric values.
  • Improvements
    • Added scrolling capability for the calendar view when content exceeds defined height.
    • Simplified rendering logic for timeline and schedule views in the calendar component.
  • Tests
    • Updated test cases for selecting specific day elements in the calendar view to ensure accurate targeting.

Copy link

coderabbitai bot commented Nov 14, 2024

Walkthrough

The changes in this pull request focus on enhancing the calendar-view component across several files. The height property type has been modified to allow both string and number values, improving flexibility. Additionally, the CSS for the calendar view has been updated to include an overflow: auto; property, enabling scrolling for overflowing content. The Vue component's template has also been adjusted to conditionally apply styles based on the height property type, streamlining the rendering logic.

Changes

File Path Change Summary
examples/sites/demos/apis/calendar-view.js Updated height property type from string to `string
packages/theme/src/calendar-view/index.less Added overflow: auto; to the main calendar view class for scrolling when content overflows.
packages/vue/src/calendar-view/src/index.ts Changed height property type in calendarViewProps from String to [String, Number].
packages/vue/src/calendar-view/src/pc.vue Adjusted template to conditionally apply height styles based on height prop type; cleaned up rendering logic.

Possibly related PRs

Suggested reviewers

  • zzcr

🐇 In the garden where the calendars grow,
A height change here, a style change there, oh!
With overflow set, the content can scroll,
Flexibility blooms, making it all whole.
Hopping through code, we make it just right,
Our calendar's ready, oh what a delight! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6abbcdf and 68f1db2.

📒 Files selected for processing (1)
  • examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.spec.ts (1 hunks)
🔇 Additional comments (2)
examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.spec.ts (2)

9-11: LGTM: Assertions are correctly maintained

The assertions checking for disabled states and date classes remain appropriate and aligned with the component's expected behavior.


6-8: Verify the robustness of the new day selectors

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 successful

Let 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:

  1. The component uses a disabled function that affects all dates before the 15th, which explains why we need specific nth selectors for dates 10, 14, and 15
  2. Other calendar tests in the same directory also consistently use nth selectors for date selection
  3. The selector changes align with the component's rendering logic where:
    • Dates before 15 are disabled (matching the disabled function implementation)
    • Multiple instances of the same date can appear due to the calendar grid structure

The nth-based selectors are currently the most reliable way to target specific dates in this calendar implementation, as:

  • The dates appear multiple times due to the calendar's month-view structure
  • The component doesn't expose more specific attributes for testing
  • Other calendar tests in the codebase follow the same pattern
🏁 Scripts executed

The 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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the bug Something isn't working label Nov 14, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 string to string | number follows 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 pcDemo and mfDemo fields 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-first

Length 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.vue

Length 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.vue

Length of output: 1492

packages/vue/src/calendar-view/src/pc.vue (1)

136-136: Consider adding overflow handling for timeline and schedule views

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7ad17d8 and 6abbcdf.

📒 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

packages/vue/src/calendar-view/src/index.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant