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: when opening the tool separately, there is no need to pass the toolbar-tools slot #4841

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

anncwb
Copy link
Collaborator

@anncwb anncwb commented Nov 9, 2024

Description

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

Release Notes

  • New Features

    • Enhanced documentation for the Vben Form component, including detailed usage examples and configuration options.
    • Introduction of a new header property in the Drawer component for improved header visibility control.
  • Improvements

    • Updated icon management guidelines, emphasizing the use of Iconify and SVG icons with clear examples.
    • Type safety improvements across various components by introducing the ClassType type for class properties.
  • Style Changes

    • New CSS rules added for toolbar styling in the vxe-table component.
  • Bug Fixes

    • Adjusted toolbar configuration logic in the grid component to improve usability.

@anncwb anncwb requested review from vince292007 and a team as code owners November 9, 2024 05:59
Copy link

changeset-bot bot commented Nov 9, 2024

⚠️ No Changeset found

Latest commit: cae9e4c

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 Nov 9, 2024

Walkthrough

The pull request introduces extensive updates to documentation and TypeScript type definitions across several components. Key changes include enhancements to the Vben Form component documentation, detailing usage, configuration, and validation examples. Type definitions are refined to improve type safety, particularly with the introduction of a new ClassType. Additionally, several components have updated properties to utilize this new type, enhancing clarity and usability throughout the UI kit.

Changes

File Change Summary
docs/src/components/common-ui/vben-form.md Updated documentation for Vben Form, detailing usage, configuration, and validation examples. Refined API section and props table.
docs/src/guide/essentials/icons.md Clarified icon management and usage, introduced categories for Iconify and SVG icons, and updated Tailwind CSS icon recommendations.
packages/@core/base/typings/src/basic.d.ts Added new type ClassType and updated export statement to include it.
packages/@core/ui-kit/form-ui/package.json Added dependency for @vben-core/typings.
packages/@core/ui-kit/form-ui/src/types.ts Updated actionWrapperClass type from any to ClassType.
packages/@core/ui-kit/popup-ui/package.json Added dependency for @vben-core/typings.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts Updated DrawerProps properties to use ClassType and added a new header property.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue Introduced header property for conditional rendering of the header section.
packages/@core/ui-kit/shadcn-ui/src/components/avatar/avatar.vue Updated class and dotClass properties to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/components/context-menu/context-menu.vue Updated class, contentClass, and itemClass properties to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/components/hover-card/hover-card.vue Updated class and contentClass properties to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/components/popover/popover.vue Updated class and contentClass properties to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/components/scrollbar/scrollbar.vue Updated class and scrollBarClass properties to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/components/tooltip/tooltip.vue Updated contentClass property to use ClassType.
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue Updated class and closeClass properties to use ClassType.
packages/effects/common-ui/src/components/captcha/types.ts Updated class property in SliderCaptchaProps to use ClassType.
packages/effects/plugins/src/vxe-table/theme.css Added new CSS rules for .vxe-toolbar and modified .vxe-tools--operate.
packages/effects/plugins/src/vxe-table/types.ts Updated class and gridClass properties in VxeGridProps to use ClassType.
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue Modified toolbar management logic and adjusted pagerConfig and formConfig settings.
playground/src/views/examples/vxe-table/basic.vue Removed class="mr-2" from a button element.

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • vince292007

🐰 In the garden of code, we hop and play,
With forms and types, we brighten the day.
New rules for classes, clear paths we trace,
Documentation blooms, a joyful embrace.
So let’s celebrate, with a leap and a cheer,
For changes that make our components clear! 🌼✨


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

🧹 Outside diff range and nitpick comments (13)
packages/@core/base/typings/src/basic.d.ts (1)

33-33: Consider enhancing type safety and clarity of ClassType.

The type could be more specific about the allowed object shape and have a more descriptive name since it appears to be used for styling classes.

Consider these improvements:

-type ClassType = Array<object | string> | object | string;
+type StyleClassType = Array<Record<string, boolean | undefined> | string> | Record<string, boolean | undefined> | string;

This would:

  • Better describe the type's purpose through naming
  • Constrain objects to typical class object patterns (e.g., { 'my-class': true })
  • Maintain compatibility with various styling approaches (strings, objects, arrays)
packages/@core/ui-kit/shadcn-ui/src/components/avatar/avatar.vue (1)

Line range hint 36-37: Consider refactoring class binding implementation

The class prop is currently bound to both the container div and the Avatar component, which could lead to unexpected styling behavior. Consider consolidating the class binding to avoid potential conflicts.

Here's a suggested improvement:

-  <div :class="props.class" class="relative flex flex-shrink-0 items-center">
-    <Avatar :class="props.class" class="size-full">
+  <div class="relative flex flex-shrink-0 items-center">
+    <Avatar :class="[props.class, 'size-full']">

This change:

  1. Removes duplicate class binding
  2. Keeps the class customization on the Avatar component where it's more likely to be needed
  3. Maintains the layout structure with the container div
packages/@core/ui-kit/shadcn-ui/src/components/hover-card/hover-card.vue (1)

16-17: LGTM: Props type enhancement with documentation suggestion

The type refinement from implicit any to ClassType improves type safety. Consider adding JSDoc comments to document the expected values for these props, especially since they're used for styling customization.

Add documentation like this:

interface Props extends HoverCardRootProps {
+  /** CSS class(es) to apply to the hover card wrapper */
  class?: ClassType;
+  /** CSS class(es) to apply to the hover card content */
  contentClass?: ClassType;
  contentProps?: HoverCardContentProps;
}
docs/src/guide/essentials/icons.md (2)

70-70: Consider enhancing the Tailwind CSS icons section structure.

The Tailwind CSS section could benefit from additional context and organization to match the depth of the Iconify and SVG sections. Consider adding:

  • Benefits and use cases for Tailwind CSS icons
  • Performance implications
  • Best practices for usage

Line range hint 31-39: Consider adding more comprehensive examples.

While the current examples are good, consider adding:

  • Examples with different icon sizes
  • Examples of common icon customizations (color, rotation, etc.)
  • Error handling examples

Example addition:

<template>
  <!-- Size variations -->
  <MdiKeyboardEsc class="size-4" /> <!-- Small -->
  <MdiKeyboardEsc class="size-6" /> <!-- Medium -->
  
  <!-- Customizations -->
  <MdiKeyboardEsc class="size-5 text-blue-500 rotate-90" />
</template>

Also applies to: 52-60

packages/@core/ui-kit/shadcn-ui/src/components/context-menu/context-menu.vue (1)

26-27: Great type safety improvement!

The replacement of any with ClassType for class-related props enhances type safety and maintainability. The consistent application across all class props (class, contentClass, itemClass) shows good attention to detail.

Consider adding JSDoc comments to document the expected format of these class props, especially since ClassType can accept multiple formats (string, object, or array). Example:

+/**
+ * CSS classes to be applied to the root element
+ * @type {string | object | (string | object)[]}
+ */
class?: ClassType;

Also applies to: 30-30

packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)

23-24: LGTM with enhancement suggestion

The type safety improvement for class and closeClass props is good. Consider adding JSDoc comments to document these props:

+/** Custom class names for the dialog content */
 class?: ClassType;
+/** Custom class names for the close button */
 closeClass?: ClassType;

Missing test coverage for DialogContent component transitions

The verification reveals that while there are test files for other UI components (form-ui, drawer, modal), there are no dedicated tests for the shadcn-ui DialogContent component. Specifically:

  • No test files exist in the shadcn-ui directory
  • No dialog-specific tests were found in the broader ui-kit scope
  • The component implements important transition and animation logic that should be tested

Action items:

  • Create test coverage for DialogContent.vue focusing on:
    • Transition and animation behaviors
    • Event forwarding
    • Interaction with DialogOverlay
    • Integration with radix-vue dialog primitives
🔗 Analysis chain

Line range hint 1-24: Verify test coverage for dialog transitions and animations

The component handles complex interactions including transitions, animations, and event forwarding. Let's verify the test coverage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for existing test files and coverage

# Look for test files related to DialogContent
fd -e spec.ts -e test.ts -e spec.tsx -e test.tsx dialog

# Check for animation/transition test coverage
rg -l "describe.*DialogContent.*transition|test.*DialogContent.*animation" -g "*.{spec,test}.{ts,tsx}"

Length of output: 162


Script:

#!/bin/bash
# Let's try a broader search to understand the test structure

# Find any test files in the ui-kit directory
fd -e spec.ts -e test.ts -e spec.tsx -e test.tsx . packages/@core/ui-kit/

# Look for any dialog-related tests with a broader pattern
rg -l "dialog|Dialog" -g "*.{spec,test}.{ts,tsx}" packages/@core/ui-kit/

# Check if there's a test directory specifically for shadcn-ui components
fd -t d "test|__tests__" packages/@core/ui-kit/shadcn-ui/

# Look for any transition/animation testing utilities or helpers
rg -l "transition|animation.*test" packages/@core/ui-kit/

Length of output: 3279

packages/effects/common-ui/src/components/captcha/types.ts (1)

77-77: Add JSDoc documentation for the class property.

While the type change to ClassType is a good improvement, consider adding JSDoc documentation to maintain consistency with other properties in the interface.

+  /**
+   * @description CSS class to be applied to the component
+   */
   class?: ClassType;
packages/@core/ui-kit/shadcn-ui/src/components/scrollbar/scrollbar.vue (1)

11-13: LGTM: Enhanced type safety for class-related props.

The migration from any to ClassType for class and scrollBarClass properties improves type safety and maintains consistency with other components in the project.

Consider adding JSDoc comments to document the expected values and usage of these props, especially since they're using a custom type. For example:

 interface Props {
+  /** CSS classes to be applied to the root element. Accepts string, object, or array of classes */
   class?: ClassType;
   horizontal?: boolean;
+  /** CSS classes to be applied to the scrollbar element */
   scrollBarClass?: ClassType;
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)

Line range hint 135-145: Consider using v-if instead of v-show for better accessibility.

The current implementation uses v-show to toggle header visibility, which keeps the DOM elements in place but hidden. This approach might cause issues:

  1. Screen readers might still interact with hidden elements
  2. Important context from title/description could be missed
  3. Unnecessary DOM elements are maintained when hidden

Consider this alternative implementation:

      <SheetHeader
-        v-show="showHeader"
+        v-if="showHeader"
        :class="
          cn(
            '!flex flex-row items-center justify-between border-b px-6 py-5',
            headerClass,
            {
              'px-4 py-3': closable,
            },
          )
        "
      >

Additionally, consider adding aria-label to the SheetContent when header is hidden:

    <SheetContent
      :class="
        cn('flex w-[520px] flex-col', drawerClass, {
          '!w-full': isMobile,
        })
      "
      :modal="modal"
      :open="state?.isOpen"
+     :aria-label="!showHeader && title ? title : undefined"
      @close-auto-focus="handleFocusOutside"
packages/@core/ui-kit/form-ui/src/types.ts (1)

307-307: LGTM! Type safety improvement.

Good improvement replacing any with ClassType for better type checking and IDE support.

Consider adding a JSDoc comment to document the expected structure of ClassType for better developer experience:

  /**
   * 表单操作区域class
+  * @type {ClassType} - Defines the class structure for the form action wrapper
   */
  actionWrapperClass?: ClassType;
docs/src/components/common-ui/vben-form.md (1)

Line range hint 1-24: Documentation enhancement suggestions

The documentation is well-structured and comprehensive, but could be enhanced with:

  1. More inline comments in code examples to explain key concepts
  2. Additional context around internationalization usage
  3. More complex validation examples showcasing real-world scenarios

Would you like me to provide specific examples for these enhancements?

Also applies to: 323-323

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9cf0573 and cae9e4c.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (20)
  • docs/src/components/common-ui/vben-form.md (1 hunks)
  • docs/src/guide/essentials/icons.md (1 hunks)
  • packages/@core/base/typings/src/basic.d.ts (1 hunks)
  • packages/@core/ui-kit/form-ui/package.json (1 hunks)
  • packages/@core/ui-kit/form-ui/src/types.ts (2 hunks)
  • packages/@core/ui-kit/popup-ui/package.json (1 hunks)
  • packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (3 hunks)
  • packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/avatar/avatar.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/context-menu/context-menu.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/hover-card/hover-card.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/popover/popover.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/scrollbar/scrollbar.vue (1 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/tooltip/tooltip.vue (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2 hunks)
  • packages/effects/common-ui/src/components/captcha/types.ts (2 hunks)
  • packages/effects/plugins/src/vxe-table/theme.css (1 hunks)
  • packages/effects/plugins/src/vxe-table/types.ts (2 hunks)
  • packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (0 hunks)
  • playground/src/views/examples/vxe-table/basic.vue (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/effects/plugins/src/vxe-table/use-vxe-grid.vue
✅ Files skipped from review due to trivial changes (1)
  • playground/src/views/examples/vxe-table/basic.vue
🔇 Additional comments (28)
packages/@core/base/typings/src/basic.d.ts (1)

35-35: LGTM!

The export statement correctly includes the new type while maintaining existing exports.

packages/@core/ui-kit/shadcn-ui/src/components/tooltip/tooltip.vue (2)

2-2: LGTM! Type import addition improves type safety.

The addition of the ClassType import aligns with the project's effort to enhance type safety across components.


Line range hint 1-38: Verify alignment with PR objectives.

The changes in this file focus on type safety improvements, but the PR description mentions fixing an issue with the toolbar-tools slot. Could you clarify if there are additional changes needed to address the toolbar-tools slot issue mentioned in the PR description?

Let's search for toolbar-related files that might need changes:

packages/@core/ui-kit/shadcn-ui/src/components/avatar/avatar.vue (2)

2-2: LGTM! Type import enhances type safety

The addition of the ClassType import aligns with the project's goal of improving type safety and reducing the use of any types.


15-17: Props type definitions look good

The update to use ClassType for class and dotClass properties improves type safety while maintaining compatibility with Vue's class binding syntax.

Let's verify the ClassType definition and its usage across the codebase:

✅ Verification successful

Props type definitions are correctly implemented

The verification confirms that:

  • ClassType is properly defined in packages/@core/base/typings/src/basic.d.ts as type ClassType = Array<object | string> | object | string
  • The type is correctly imported from @vben-core/typings in avatar.vue
  • The usage of ClassType for class and dotClass props aligns with the type definition and is consistent with other components in the codebase that have adopted this type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify ClassType definition and its usage pattern
# Expected: Find the type definition and confirm consistent usage

# Check the ClassType definition
rg -A 5 "type ClassType" packages/@core/typings

# Verify similar usage pattern in other components
rg -l "ClassType.*class\?:" packages/@core/ui-kit

Length of output: 164


Script:

#!/bin/bash
# Let's try to find the ClassType definition in the entire codebase
rg "type\s+ClassType"

# Also search for any imports of ClassType
rg "import.*ClassType"

# Check the specific file for the import statement
cat "packages/@core/ui-kit/shadcn-ui/src/components/avatar/avatar.vue"

# Look for similar class type patterns in other components
rg "class\?:\s*[A-Za-z]+" packages/@core/ui-kit

Length of output: 14683

packages/@core/ui-kit/popup-ui/package.json (1)

44-44: LGTM! Verify usage of the new dependency.

The addition of @vben-core/typings is properly organized and follows the workspace dependency pattern.

Let's verify that this dependency is being used in the codebase:

✅ Verification successful

The dependency is properly used in the codebase

The @vben-core/typings dependency is actively used in the codebase. Found usage in:

  • packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts: Imports and uses ClassType for type definitions in multiple properties:
    import type { ClassType } from '@vben-core/typings';
    
    class?: ClassType;
    footerClass?: ClassType;
    headerClass?: ClassType;

The dependency addition is valid and necessary for the type definitions in the drawer component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of @vben-core/typings in popup-ui package
# Expected: Find import statements or type references from @vben-core/typings

# Search for imports from @vben-core/typings in the popup-ui source files
rg -t ts -t vue "from ['|\"]@vben-core/typings" "packages/@core/ui-kit/popup-ui/src"

# Search for type references using the new ClassType
rg -t ts -t vue "ClassType" "packages/@core/ui-kit/popup-ui/src"

Length of output: 205


Script:

#!/bin/bash
# Let's try a different approach without file type filters
# First, check if the directory exists and list its contents
ls -la packages/@core/ui-kit/popup-ui/src/

# Then search for imports and type usage without file type filters
rg "from ['|\"]@vben-core/typings" packages/@core/ui-kit/popup-ui/src/
rg "ClassType" packages/@core/ui-kit/popup-ui/src/

# Also check TypeScript/Vue files specifically
fd "\.(ts|vue)$" packages/@core/ui-kit/popup-ui/src/

# If we find any files, let's look at their content to check for type imports
fd "\.(ts|vue)$" packages/@core/ui-kit/popup-ui/src/ -x cat {}

Length of output: 42342

packages/@core/ui-kit/shadcn-ui/src/components/popover/popover.vue (2)

2-2: LGTM! Type import enhances type safety

The addition of ClassType import aligns with the project's effort to improve type safety across components.


20-21: Add JSDoc comments for improved documentation

While the type changes from any to ClassType improve type safety, consider adding JSDoc comments to document the expected values and usage of these props.

Let's verify the consistent usage of ClassType across similar components:

Suggested documentation:

 interface Props extends PopoverRootProps {
+  /** CSS class(es) to apply to the popover root element */
   class?: ClassType;
+  /** CSS class(es) to apply to the popover content element */
   contentClass?: ClassType;
   contentProps?: PopoverContentProps;
 }
packages/effects/plugins/src/vxe-table/types.ts (1)

1-1: LGTM: Clean import addition

The addition of ClassType import is well-organized alongside existing type imports and supports the type safety improvements.

packages/@core/ui-kit/shadcn-ui/src/components/hover-card/hover-card.vue (1)

2-2: LGTM: Type import addition enhances type safety

The addition of ClassType import aligns with the codebase-wide initiative to standardize type definitions.

packages/@core/ui-kit/form-ui/package.json (1)

43-43: LGTM! Verify dependency updates in consuming packages.

The addition of @vben-core/typings as a workspace dependency is a positive change that supports the broader effort to improve type safety across components.

Let's verify that all consuming packages have been updated:

✅ Verification successful

All consuming packages have @vben-core/typings dependency properly configured

The verification shows that all packages importing from @vben-core/typings have correctly included it as a workspace dependency in their package.json files. This includes:

  • packages/types
  • packages/utils
  • packages/@core/ui-kit/tabs-ui
  • packages/@core/ui-kit/shadcn-ui
  • packages/@core/ui-kit/popup-ui
  • packages/@core/ui-kit/menu-ui
  • packages/@core/ui-kit/layout-ui
  • packages/@core/ui-kit/form-ui
  • packages/@core/preferences

The dependency is consistently declared as "@vben-core/typings": "workspace:*" across all packages, ensuring proper version management within the monorepo.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any packages that might need updates to include @vben-core/typings

# Test: Find package.json files that use types from @vben-core/typings but don't list it as a dependency
rg -l "from ['|\"]@vben-core/typings" | while read -r file; do
  dir=$(dirname "$file")
  if [ -f "$dir/package.json" ]; then
    if ! grep -q "@vben-core/typings" "$dir/package.json"; then
      echo "Package at $dir might need @vben-core/typings dependency"
    fi
  fi
done

Length of output: 6992


Script:

#!/bin/bash
# Let's check which packages are actually using @vben-core/typings imports
# and verify their package.json dependencies

# First, get all files importing from @vben-core/typings
echo "Files importing from @vben-core/typings:"
rg "from ['|\"]@vben-core/typings" -l

# Then, for each unique package containing these files, check its package.json
echo -e "\nChecking package.json files:"
rg "from ['|\"]@vben-core/typings" -l | while read -r file; do
    # Get the package directory by going up until we find package.json
    dir=$(dirname "$file")
    while [[ "$dir" != "." && ! -f "$dir/package.json" ]]; do
        dir=$(dirname "$dir")
    done
    
    if [[ -f "$dir/package.json" ]]; then
        echo -e "\nPackage: $dir"
        echo "Dependencies containing @vben-core/typings:"
        grep -A 1 -B 1 "@vben-core/typings" "$dir/package.json" || echo "No @vben-core/typings dependency found!"
    fi
done

Length of output: 38749

docs/src/guide/essentials/icons.md (1)

Line range hint 31-33: Verify import paths and package structure.

The import paths look correct, but let's verify the package structure to ensure the documentation matches the actual implementation.

Also applies to: 52-54

✅ Verification successful

Import path in documentation is correct and verified

The documentation accurately reflects the implementation:

  • @vben/icons is the correct package name as shown in package.json
  • MdiKeyboardEsc icon is properly exported from the package in packages/icons/src/iconify/index.ts
  • The import statement import { MdiKeyboardEsc } from '@vben/icons' matches the actual implementation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the existence of the icon directories and files
echo "Checking icon directories..."
fd -t d "icons" packages/
echo "Checking specific import paths..."
fd -t f "index.ts" packages/icons/src/

Length of output: 534


Script:

#!/bin/bash
# Check the exports from the icons package
echo "Checking exports from icons package..."
rg "export.*MdiKeyboardEsc" packages/icons/ -A 2

# Check if the icon is defined in the package
echo "Checking for MdiKeyboardEsc definition..."
rg "MdiKeyboardEsc" packages/icons/ --type ts

# Check package.json to verify the package name
echo "Checking package.json..."
cat packages/icons/package.json | jq '.name'

Length of output: 735

packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (3)

1-1: LGTM: Clean import addition for type safety improvements

The addition of ClassType import aligns with the type safety improvements across the codebase.


Line range hint 89-108: Consider updating related interfaces for consistency

The header property has been added to DrawerProps, but DrawerState and DrawerApiOptions interfaces might need to be reviewed to ensure they properly handle this new property through inheritance.

Let's check for any similar patterns in other components:

#!/bin/bash
# Description: Check how other components handle similar state/options inheritance

# Search for similar interface inheritance patterns
ast-grep --pattern 'interface $name extends $base {
  $$$
}'

51-55: Please clarify the relationship with PR objective

While the header property addition is well-documented, its relationship to the PR's objective of "fixing the toolbar-tools slot when opening the tool separately" isn't immediately clear. Could you please explain:

  1. How does this header property relate to the toolbar-tools slot issue?
  2. Are there any potential breaking changes for existing implementations?

Let's check for existing usage:

#!/bin/bash
# Description: Check for existing usage of the drawer component with header-related props

# Search for drawer component usage with header prop
rg -A 5 "<(?:VbenDrawer|Drawer)[^>]*header=" --type vue

# Search for toolbar-tools slot usage
rg -A 5 "toolbar-tools" --type vue
packages/@core/ui-kit/shadcn-ui/src/components/context-menu/context-menu.vue (2)

2-2: LGTM: Clean type import addition

The import of ClassType from the core typings is well-placed and follows proper TypeScript practices.


26-27: Verify class binding compatibility

While the type changes look good, we should verify that all existing usages of these props comply with the new ClassType constraints.

Also applies to: 30-30

✅ Verification successful

Let me gather more information about the ClassType definition and usage to ensure compatibility.

The previous results show the context menu is used in the tabbar component with class bindings, so we need to verify the type compatibility. Let's check the type definitions first to make a proper assessment.


Class type constraints are compatible with existing usage

The verification shows that:

  1. ClassType is a flexible type that accepts arrays of objects/strings, objects, or strings - which covers all common Vue class binding patterns
  2. The type is consistently used across multiple UI components in the codebase
  3. The actual usage in tabbar.vue shows standard Vue class bindings that comply with the type constraints
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of context-menu component with class-related props
echo "Checking context-menu usage patterns..."
rg -l "context-menu" | xargs rg -A 2 -B 2 "(:class|v-bind:class|class=)"

# Search for any direct imports of this component
echo "Checking component imports..."
rg -l "from.*context-menu.vue"

Length of output: 3175


Script:

#!/bin/bash
# Check the definition of ClassType
echo "Checking ClassType definition..."
rg -A 5 "type\s+ClassType"

# Check other components using ClassType
echo "Checking other components using ClassType..."
ast-grep --pattern 'type ClassType'

# Check for any type imports related to ClassType
echo "Checking type imports..."
rg "import.*ClassType"

Length of output: 1861

packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (1)

2-2: LGTM: Type-only import for enhanced type safety

The addition of ClassType import improves type safety for class-related properties.

packages/effects/common-ui/src/components/captcha/types.ts (2)

1-1: LGTM! Good type safety improvement.

The type-only import of ClassType aligns with TypeScript best practices and the project's goal of improving type safety.


Line range hint 1-77: Verify alignment with PR objectives.

The changes in this file focus on type safety improvements (changing from any to ClassType), but the PR description mentions fixing an issue with the toolbar-tools slot. Could you clarify how these type changes relate to the PR's objective?

packages/effects/plugins/src/vxe-table/theme.css (2)

91-93: LGTM: Toolbar padding adjustment aligns with PR objective.

The removal of top padding helps maintain consistent spacing when the toolbar is opened separately.


95-97: Consider browser compatibility for :not(:has()) selector.

While the margin adjustment for empty tool operates is logically sound, the :has() selector is a relatively new CSS feature. Consider adding a fallback for older browsers.

Let's check the browser support requirements in the project:

Consider adding a fallback using a CSS custom property:

+.vxe-tools--operate {
+  margin-left: 0; /* Fallback */
+  margin-left: var(--tools-operate-margin, 0);
+}

 .vxe-tools--operate:not(:has(button)) {
-  margin-left: 0;
+  --tools-operate-margin: 0;
 }
✅ Verification successful

Let me analyze the browser support requirements from the package.json files. I don't see any explicit browser support configuration (no .browserslistrc or browserslist field in package.json files). Let me check if there are any other relevant configuration files or documentation that might indicate browser support requirements.


Based on the README documentation, I can now provide a final response since I found the browser support requirements for the project.

The :has() selector usage is compatible with the project's browser requirements

The project explicitly states it supports "last 2 versions" of modern browsers (Chrome, Firefox, Edge, Safari) and doesn't support IE. The :has() selector is well-supported in all these modern browsers:

  • Chrome 105+ (Sept 2022)
  • Firefox 103+ (July 2022)
  • Safari 15.4+ (March 2022)
  • Edge 105+ (Sept 2022)

Since the project targets the last 2 versions of modern browsers and we're in 2024, the :has() selector is safe to use without fallbacks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for browser support documentation or configuration
fd -t f "^(package\.json|browserslist|\.browserslistrc)$" --exec cat {}

Length of output: 46563


Script:

#!/bin/bash
# Search for any browser support related configuration or documentation
fd -t f "^(README|CONTRIBUTING|BROWSER_SUPPORT)\.(md|txt)$" --exec cat {}

Length of output: 10174

packages/@core/ui-kit/shadcn-ui/src/components/scrollbar/scrollbar.vue (2)

2-2: LGTM: Type import addition is appropriate.

The addition of the ClassType import aligns with the project's initiative to improve type safety across components.


2-2: Verify consistent ClassType implementation across components.

Let's ensure the ClassType implementation is consistent with its usage across other components.

Also applies to: 11-13

✅ Verification successful

Based on the results, I can now generate the final response:

ClassType usage is consistent across components

The verification shows that:

  • ClassType is properly exported from @core/base/typings
  • It's consistently used across multiple UI components for styling props
  • The implementation pattern is uniform, typically used for class-related properties like class, contentClass, dotClass, etc.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check ClassType definition and usage patterns
echo "Checking ClassType definition:"
rg -A 5 "export.*ClassType" 

echo -e "\nChecking ClassType usage patterns across components:"
rg -A 2 "class.*:.*ClassType"

Length of output: 3744

packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (2)

60-62: LGTM! Clean addition of the header visibility control.

The new header property is well-integrated into the existing usePriorityValues destructuring, following the component's established patterns for visibility controls.


60-62: Verify documentation and usage of the new header property.

The addition of the header property needs to be properly documented and its usage verified across the codebase.

Let's check for documentation and usage:

Also applies to: 135-145

✅ Verification successful

Based on the documentation found, I can now generate the final response:

The header property is properly documented and matches the implementation

The documentation in docs/src/components/common-ui/vben-drawer.md clearly shows that the header property is documented as a boolean prop with the description "显示header" (show header). This matches the implementation in the code where header: showHeader is being used.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for documentation updates
echo "Checking for documentation updates..."
rg -l "header.*boolean.*drawer" docs/

# Check for existing usage of the drawer component with header prop
echo "Checking for drawer component usage..."
rg -A 2 "<(?i)drawer[^>]*header="

Length of output: 247


Script:

#!/bin/bash
# Let's try a more comprehensive search strategy

# 1. Find all documentation files related to drawer
echo "=== Searching for drawer documentation ==="
fd -e md -e mdx drawer

# 2. Check the drawer component's props definition
echo -e "\n=== Checking drawer component props ==="
rg -A 5 "props:\s*{" packages/@core/ui-kit/popup-ui/src/drawer/

# 3. Look for drawer usage with any header-related props
echo -e "\n=== Checking drawer usage patterns ==="
rg -l "<drawer[\s\S]*?>" --type vue

# 4. Check for any header-related props in test files
echo -e "\n=== Checking test files ==="
fd -e spec.js -e test.js drawer

Length of output: 673


Script:

#!/bin/bash
# Let's fix the search patterns and try again

# 1. Check the content of the drawer documentation
echo "=== Checking drawer documentation content ==="
cat docs/src/components/common-ui/vben-drawer.md

# 2. Check the drawer component's props definition with fixed regex
echo -e "\n=== Checking drawer component props ==="
rg "props:" -A 5 packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue

# 3. Look for drawer usage with fixed pattern
echo -e "\n=== Checking drawer usage patterns ==="
rg -l -t vue "<drawer"

# 4. If we find any usage, let's examine the first few
echo -e "\n=== Examining drawer usage examples ==="
rg -A 2 "<drawer" $(rg -l -t vue "<drawer")

Length of output: 3956

packages/@core/ui-kit/form-ui/src/types.ts (1)

2-2: LGTM! Clean type-only import.

The import statement follows TypeScript best practices by using the type keyword for type-only imports.

docs/src/components/common-ui/vben-form.md (1)

323-323: LGTM: Improved type safety for class property

The change from any to ClassType enhances type safety while maintaining flexibility for various class definitions.

@anncwb anncwb merged commit 2c6edaf into main Nov 9, 2024
14 checks passed
@anncwb anncwb deleted the bugfix/table-tools branch November 9, 2024 06:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant