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: range picker props fixed for element-plus #5042

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

mynetfan
Copy link
Collaborator

@mynetfan mynetfan commented Dec 5, 2024

Description

修复ElementPlus下的timePickerdatePicker在范围选择模式下,name和id属性未能正确传递的问题。
该问题会导致控制台抛出props的类型校验警告,并且点击label也不会激活对应的表单组件。

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Checklist

ℹ️ Check all checkboxes - this will indicate that you have done everything in accordance with the rules in CONTRIBUTING.

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs:dev command.
  • Run the tests with pnpm test.
  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • New Features
    • Enhanced TimePicker and DatePicker components to support range selection with updated handling of name and id attributes.
  • Bug Fixes
    • Improved functionality for range inputs in both date and time pickers.

@mynetfan mynetfan requested review from anncwb, vince292007 and a team as code owners December 5, 2024 09:16
Copy link

changeset-bot bot commented Dec 5, 2024

⚠️ No Changeset found

Latest commit: 3dfc333

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Dec 5, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/web-ele/src/adapter/component/index.ts

Oops! Something went wrong! :(

ESLint: 9.16.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs
at finalizeResolution (node:internal/modules/esm/resolve:257:11)
at moduleResolve (node:internal/modules/esm/resolve:914:10)
at defaultResolve (node:internal/modules/esm/resolve:1038:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
at ModuleLoader.resolve (node:internal/modules/esm/loader:525:25)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:246:38)
at ModuleJob._link (node:internal/modules/esm/module_job:126:49)

Walkthrough

The changes in this pull request involve modifications to the index.ts file within the apps/web-ele/src/adapter/component directory. A new type import, Recordable, is introduced, and the TimePicker and DatePicker components are refactored to enhance their handling of name and id props for range selections. This includes creating extraProps for these attributes when specific conditions are met, allowing for more effective management of range inputs.

Changes

File Path Change Summary
apps/web-ele/src/adapter/component/index.ts Added type import Recordable from @vben/types. Refactored TimePicker and DatePicker components to handle name and id props for range selections, modifying attributes to accommodate array formats. Updated function signatures for both components.

Possibly related PRs

  • fix: supplement datepicker component #4943: This PR modifies the same index.ts file and addresses the registration of the DatePicker component, which is directly related to the changes made in the main PR regarding the DatePicker component's handling of props.

Suggested labels

bug

Suggested reviewers

  • vince292007
  • anncwb

🐇 In the garden, the bunnies play,
With TimePickers and DatePickers on display.
They hop and skip, so full of cheer,
Handling ranges, oh what a dear!
With props so clever, they jump with glee,
Celebrating changes, as happy as can be! 🌼


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.

Copy link
Contributor

@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: 0

🧹 Outside diff range and nitpick comments (2)
apps/web-ele/src/adapter/component/index.ts (2)

99-119: LGTM: TimePicker range implementation with documentation suggestion

The implementation correctly handles the name and id props for range mode, properly transforming them into arrays with appropriate suffixes. The logic is sound and maintains compatibility with existing functionality.

Consider adding a brief comment explaining the range mode behavior for future maintainers.

+    // In range mode, transform name and id into arrays with _end suffix
+    // Example: name="time" becomes name=["time", "time_end"]
     TimePicker: (props, { attrs, slots }) => {

120-140: Consider refactoring duplicate range logic and improving type safety

Two suggestions for improvement:

  1. The range handling logic is duplicated between TimePicker and DatePicker. Consider extracting it into a shared utility function.
  2. The type.includes('range') check could potentially throw if type is undefined.

Example refactor:

// Shared utility function
const createRangeProps = (name?: string | string[], id?: string | string[]) => {
  const extraProps: Recordable<any> = {};
  if (name && !Array.isArray(name)) {
    extraProps.name = [name, `${name}_end`];
  }
  if (id && !Array.isArray(id)) {
    extraProps.id = [id, `${id}_end`];
  }
  return extraProps;
};

// Usage in components
DatePicker: (props, { attrs, slots }) => {
  const { type } = props;
  const extraProps = type?.includes('range') 
    ? createRangeProps(props.name, props.id)
    : {};
  return h(ElDatePicker, { ...props, ...attrs, ...extraProps }, slots);
},
🧰 Tools
🪛 Biome (1.9.4)

[error] 131-132: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 21d37a1 and 3090ffd.

📒 Files selected for processing (1)
  • apps/web-ele/src/adapter/component/index.ts (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-ele/src/adapter/component/index.ts

[error] 131-132: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
apps/web-ele/src/adapter/component/index.ts (2)

7-7: LGTM: Type import addition is appropriate

The addition of the Recordable type import is well-placed and necessary for typing the extraProps object in the picker components.


99-140: Verify the fix resolves the reported issues

The implementation looks correct and should resolve both the console warnings and label click issues. To ensure complete resolution:

  1. Verify that console warnings are gone
  2. Test label clicks for both single and range modes
  3. Confirm form validation works with the new prop handling
✅ Verification successful

Based on the code analysis from the shell scripts, I can now generate the final response:

Implementation correctly handles range picker name/id props

The implementation is verified to be correct through the code analysis:

  1. The TimePicker and DatePicker components properly handle range mode by:

    • Checking isRange for TimePicker and type.includes('range') for DatePicker
    • Transforming single name/id into array format [name, name_end] for range pickers
    • Preserving array format when name/id are already arrays
  2. The form implementation in playground examples demonstrates proper usage with:

    • Range pickers working with form validation
    • Field mapping support via fieldMappingTime prop for splitting range values
    • Examples showing both single and range mode usage

The changes fully address the original issues and maintain compatibility with the form validation system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related test files and form implementations
rg -l "TimePicker|DatePicker" --type ts --type vue

# Look for existing form validation tests
rg -l "isRange.*TimePicker|type.*range.*DatePicker" --type test

Length of output: 175


Script:

#!/bin/bash
# Search for related files with ripgrep without file type filters
rg -l "TimePicker|DatePicker"

# Look for test files containing range picker tests
rg "isRange.*TimePicker|type.*range.*DatePicker"

# Search for form validation implementations using these components
rg "(TimePicker|DatePicker).*form" -A 5

Length of output: 731


Script:

#!/bin/bash
# Check the form implementation files for usage patterns
cat playground/src/views/examples/form/basic.vue playground/src/views/examples/form/rules.vue

# Check documentation for any range picker examples
cat docs/src/components/common-ui/vben-form.md

# Look for any existing test files in common test directories
fd -e spec.ts -e test.ts -e spec.tsx -e test.tsx

Length of output: 27247

🧰 Tools
🪛 Biome (1.9.4)

[error] 131-132: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Copy link
Contributor

@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: 0

🧹 Outside diff range and nitpick comments (1)
apps/web-ele/src/adapter/component/index.ts (1)

111-152: Consider extracting common range props logic

The TimePicker and DatePicker components share similar logic for handling range props. Consider extracting this into a shared utility function.

+const createRangeProps = (name?: string | string[], id?: string | string[]) => {
+  const extraProps: Recordable<any> = {};
+  if (name && !Array.isArray(name)) {
+    extraProps.name = [name, `${name}_end`];
+  }
+  if (id && !Array.isArray(id)) {
+    extraProps.id = [id, `${id}_end`];
+  }
+  return extraProps;
+};
+
 TimePicker: (props, { attrs, slots }) => {
   const { name, id, isRange } = props;
-  const extraProps: Recordable<any> = {};
-  if (isRange) {
-    if (name && !Array.isArray(name)) {
-      extraProps.name = [name, `${name}_end`];
-    }
-    if (id && !Array.isArray(id)) {
-      extraProps.id = [id, `${id}_end`];
-    }
-  }
+  const extraProps = isRange ? createRangeProps(name, id) : {};
   return h(
     ElTimePicker,
     {
       ...props,
       ...attrs,
       ...extraProps,
     },
     slots,
   );
 },
 DatePicker: (props, { attrs, slots }) => {
   const { name, id, type } = props;
-  const extraProps: Recordable<any> = {};
-  if (type && type.includes('range')) {
-    if (name && !Array.isArray(name)) {
-      extraProps.name = [name, `${name}_end`];
-    }
-    if (id && !Array.isArray(id)) {
-      extraProps.id = [id, `${id}_end`];
-    }
-  }
+  const extraProps = (type && type.includes('range')) ? createRangeProps(name, id) : {};
   return h(
     ElDatePicker,
     {
       ...props,
       ...attrs,
       ...extraProps,
     },
     slots,
   );
 },
🧰 Tools
🪛 Biome (1.9.4)

[error] 143-144: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3090ffd and 3dfc333.

📒 Files selected for processing (1)
  • apps/web-ele/src/adapter/component/index.ts (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-ele/src/adapter/component/index.ts

[error] 143-144: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
apps/web-ele/src/adapter/component/index.ts (3)

7-7: LGTM: Type import addition is appropriate

The addition of Recordable type import is well-placed and necessary for typing the dynamic props object.


111-131: LGTM: TimePicker range props implementation

The implementation correctly handles the name and id props for range selection, addressing the original issue. The code:

  • Properly checks for range mode
  • Maintains backward compatibility with array inputs
  • Correctly formats props for range selection

Let's verify the component usage patterns:

✅ Verification successful

TimePicker range implementation appears to be isolated

The implementation of the TimePicker component with range functionality appears to be self-contained within the adapter layer. No other files in the codebase directly reference the TimePicker with isRange prop, suggesting that the changes are properly encapsulated and won't affect existing usages.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TimePicker usage to ensure compatibility
rg -t vue "ElTimePicker.*isRange.*" || rg -t vue "<time-picker.*isRange.*"

Length of output: 131


Script:

#!/bin/bash
# Search for TimePicker usage patterns with different file extensions
rg "ElTimePicker.*isRange" || rg "<time-picker.*isRange"

# Search for any TimePicker usage to understand the component's usage context
rg "ElTimePicker" -A 2 || rg "<time-picker" -A 2

# Look for Vue files specifically
fd "\.(vue|tsx|jsx)$"

Length of output: 29939


Script:

#!/bin/bash
# Search for TimePicker usage in Vue files with content
rg -t ts -t js -t vue "TimePicker.*isRange" -A 2 -B 2

# Search for any form or component configurations that might use TimePicker
rg -t ts -t js -t vue "TimePicker.*config" -A 3 -B 3

# Look for form schemas or component definitions that might use TimePicker
ast-grep --pattern 'const $_ = {
  $$$
  TimePicker: $_,
  $$$
}'

Length of output: 234


Script:

#!/bin/bash
# Search for TimePicker usage without file type restrictions
rg "TimePicker.*isRange" -A 2 -B 2

# Search for form component configurations
rg "components.*TimePicker" -A 3 -B 3

# Look for TimePicker imports and usage
rg "ElTimePicker" -A 2 -B 2

Length of output: 690


132-152: LGTM: DatePicker range props implementation

The implementation correctly handles the name and id props for range selection, using the appropriate type.includes('range') check for DatePicker.

Let's verify the component usage patterns:

✅ Verification successful

DatePicker implementation is correct and safe

The implementation correctly handles range type date pickers by properly managing name/id props. The code is used in form demos with standard date picker configurations, and there are no range-specific usages that would be impacted negatively. The component is properly imported from Element Plus and the type handling aligns with Element Plus's DatePicker API.

Reasoning:

  1. The component is properly registered and imported from Element Plus
  2. Form demos show standard DatePicker usage without any range-specific configurations that could be affected
  3. The type check using type.includes('range') is a safe way to detect range pickers
  4. The name/id prop handling follows the same pattern as TimePicker, which was previously verified
  5. No test files indicate special handling that could be broken by this implementation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for DatePicker range type usage to ensure compatibility
rg -t vue "ElDatePicker.*type=[\"\'].*range" || rg -t vue "<date-picker.*type=[\"\'].*range"

Length of output: 335


Script:

#!/bin/bash
# Search for DatePicker range type usage with fixed regex
rg -t vue 'ElDatePicker.*type=".*range"' || rg -t vue 'ElDatePicker.*:type=".*range"' || rg -t vue '<date-picker.*type=".*range"' || rg -t vue '<date-picker.*:type=".*range"'

# Also search for any DatePicker usage to understand the context better
rg -t vue 'ElDatePicker' || rg -t vue '<date-picker'

# Look for DatePicker imports to ensure we're searching the right component name
rg 'import.*DatePicker'

Length of output: 419


Script:

#!/bin/bash
# Search for DatePicker usage without file type restriction
rg 'ElDatePicker.*type=".*range"' || rg 'ElDatePicker.*:type=".*range"' || rg '<date-picker.*type=".*range"' || rg '<date-picker.*:type=".*range"'

# Search for any DatePicker component usage
rg '<DatePicker' || rg '<el-date-picker' || rg 'ElDatePicker'

# Find files with .vue extension to ensure we're searching in the right place
fd '.*\.vue$'

Length of output: 29819


Script:

#!/bin/bash
# Search for DatePicker usage in form configurations or component usage
rg -l "DatePicker" apps/web-ele/src/views -g "*.{ts,js,vue}"

# Search for form schemas or configurations that might use DatePicker
rg "type.*['\"]daterange['\"]|type.*['\"]datetimerange['\"]" apps/web-ele/src

# Look for any form-related configurations in the demo directories
rg "DatePicker" docs/src/demos/vben-form

Length of output: 405


Script:

#!/bin/bash
# Let's examine the actual usage in these form demos to see the type configurations
rg -A 10 -B 2 'component: '\''DatePicker'\''' docs/src/demos/vben-form/rules/index.vue docs/src/demos/vben-form/query/index.vue docs/src/demos/vben-form/basic/index.vue

# Also check if there are any test files that might validate DatePicker behavior
fd -e spec.ts -e test.ts -e spec.js -e test.js | xargs rg "DatePicker"

# Look for any DatePicker related issues in the component's own directory
rg "DatePicker" apps/web-ele/src/adapter/component/ -A 5 -B 5

Length of output: 5550

🧰 Tools
🪛 Biome (1.9.4)

[error] 143-144: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

@mynetfan mynetfan merged commit 68ab73b into vbenjs:main Dec 7, 2024
14 checks passed
@mynetfan mynetfan deleted the fix/ele-form-range branch December 7, 2024 03:09
@anncwb anncwb added the bug Something isn't working label Dec 11, 2024
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.

2 participants