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

feat: add icon-picker component #4832

Merged
merged 3 commits into from
Nov 9, 2024
Merged

Conversation

DesignHhuang
Copy link
Contributor

@DesignHhuang DesignHhuang commented Nov 7, 2024

Description

feat: add icon-picker component
添加图标选择组件 fix: #4526

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

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

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced listIcons function for improved icon management.
    • Added new icons: Grip and Package2.
    • Launched IconPicker component for user-friendly icon selection with pagination support.
  • Enhancements

    • Added customizable tooltip delay for buttons.
    • Updated pagination utility for better list management.
  • Dependencies

    • Added dependency on @vben/hooks for enhanced functionality.

These updates aim to improve user experience and flexibility in icon selection and management.

Copy link

changeset-bot bot commented Nov 7, 2024

⚠️ No Changeset found

Latest commit: 32df443

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

Walkthrough

The pull request introduces several changes across multiple files, primarily adding new components and functionalities related to icon management and selection. Key updates include the addition of the listIcons function, new icons in the lucide.ts file, a customizable tooltip delay in the icon-button component, and the introduction of an icon-picker component with pagination support. Additionally, the package.json file has been updated to include a new dependency. These changes enhance the icon selection capabilities and improve component flexibility.

Changes

File Path Change Summary
packages/@core/base/icons/src/index.ts Added listIcons function to exports.
packages/@core/base/icons/src/lucide.ts Added new icons: Grip and Package2.
packages/@core/ui-kit/shadcn-ui/src/components/button/icon-button.vue Added tooltipDelayDuration prop to Props interface and updated tooltip behavior.
packages/effects/common-ui/package.json Added dependency: "@vben/hooks": "workspace:*".
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue Introduced new icon-picker component with pagination support and event emissions.
packages/effects/common-ui/src/components/icon-picker/index.ts Exported IconPicker from icon-picker.vue.
packages/effects/common-ui/src/components/index.ts Added export statement for icon-picker.
packages/effects/hooks/src/index.ts Added export statement for use-pagination module.
packages/effects/hooks/src/use-pagination.ts Introduced usePagination function for managing pagination.
playground/src/views/demos/features/icons/icon-picker.vue Added icon-picker component with defined properties.
playground/src/views/demos/features/icons/icons.data.ts Introduced data file exporting a list of icons and a prefix.
playground/src/views/demos/features/icons/index.vue Added IconPicker component to enhance icon selection functionality.

Assessment against linked issues

Objective Addressed Explanation
Add icon picker component (#4526)

Possibly related PRs

Suggested labels

feature

Suggested reviewers

  • vince292007
  • anncwb

Poem

🐰 In a world of icons bright,
A picker hops into the light.
With delays for tooltips set,
New icons added, no regret.
Choose your style, let it show,
With every click, let creativity flow! 🌟


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2a631de and 32df443.

📒 Files selected for processing (1)
  • packages/effects/common-ui/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/effects/common-ui/package.json

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 (11)
packages/effects/hooks/src/use-pagination.ts (1)

13-34: Consider enhancing the API with additional pagination metadata.

The hook could provide more useful information to consumers.

Consider adding these computed properties:

 export function usePagination<T = any>(list: Ref<T[]>, pageSize: number) {
   // ... existing code ...

+  const isFirstPage = computed(() => currentPage.value === 1);
+  const isLastPage = computed(() => 
+    currentPage.value === unref(totalPages)
+  );
+  const pageCount = computed(() => 
+    Math.ceil(unref(list).length / unref(pageSizeRef))
+  );

   return {
     setCurrentPage,
     getTotal,
     setPageSize,
     getPaginationList,
+    isFirstPage,
+    isLastPage,
+    pageCount,
+    currentPage: readonly(currentPage),
   };
 }
packages/effects/common-ui/package.json (1)

31-31: Sort dependencies alphabetically.

The dependencies should be ordered alphabetically for better maintainability.

Apply this diff to fix the ordering:

  "@vben/constants": "workspace:*",
+ "@vben/hooks": "workspace:*",
  "@vben/icons": "workspace:*",
  "@vben/locales": "workspace:*",
  "@vben/types": "workspace:*",
- "@vben/hooks": "workspace:*",
🧰 Tools
🪛 GitHub Check: Lint (ubuntu-latest)

[failure] 31-31:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/types'

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

25-25: LGTM: Default tooltip delay is reasonable.

The 200ms default delay is a good choice for tooltip behavior. Consider making this value configurable via a global theme configuration for consistency across the application.


47-51: Consider reducing template code duplication.

While the tooltip delay implementation is correct, there's an opportunity to reduce code duplication by restructuring the template.

Consider this alternative structure that avoids duplicating the VbenButton code:

-  <VbenButton
-    v-if="!showTooltip"
-    :class="cn('rounded-full', props.class)"
-    :disabled="disabled"
-    :variant="variant"
-    size="icon"
-    @click="onClick"
-  >
-    <slot></slot>
-  </VbenButton>
-
-  <VbenTooltip
-    v-else
-    :delay-duration="tooltipDelayDuration"
-    :side="tooltipSide"
-  >
+  <template v-if="showTooltip">
+    <VbenTooltip
+      :delay-duration="tooltipDelayDuration"
+      :side="tooltipSide"
+    >
+      <template #trigger>
+        <VbenButton
+          :class="cn('rounded-full', props.class)"
+          :disabled="disabled"
+          :variant="variant"
+          size="icon"
+          @click="onClick"
+        >
+          <slot></slot>
+        </VbenButton>
+      </template>
+      <slot v-if="slots.tooltip" name="tooltip"> </slot>
+      <template v-else>
+        {{ tooltip }}
+      </template>
+    </VbenTooltip>
+  </template>
+  <template v-else>
     <template #trigger>
       <VbenButton
         :class="cn('rounded-full', props.class)"
@@ -62,11 +62,8 @@
       >
         <slot></slot>
       </VbenButton>
-    </template>
-    <slot v-if="slots.tooltip" name="tooltip"> </slot>
-    <template v-else>
-      {{ tooltip }}
-    </template>
-  </VbenTooltip>
+  </template>

This refactoring:

  1. Reduces code duplication
  2. Makes the conditional logic clearer
  3. Maintains the same functionality
playground/src/views/demos/features/icons/icon-picker.vue (4)

1-9: Consider organizing imports by category.

While all imports are necessary, consider organizing them in a more structured way:

  1. Vue core imports
  2. External library imports (ant-design-vue)
  3. Internal library imports (@vben/*)
  4. Local imports
<script lang="ts" setup>
import { ref } from 'vue';

-import { IconPicker } from '@vben/common-ui';
-import { listIcons } from '@vben/icons';
-
import { Input } from 'ant-design-vue';

+import { IconPicker } from '@vben/common-ui';
+import { listIcons } from '@vben/icons';
+
import iconsData from './icons.data';

11-21: Add JSDoc documentation for all props and improve type safety.

While the interface is well-structured, consider these improvements:

  1. Add JSDoc documentation for all properties to improve maintainability
  2. Add validation for pageSize
  3. Use more specific type for width property
 export interface Props {
+  /**
+   * Whether to show clear button
+   * @default true
+   */
   allowClear?: boolean;
+  /**
+   * Number of icons to display per page
+   * @default 36
+   * @minimum 1
+   */
   pageSize?: number;
   /**
    * 可以通过prefix获取系统中使用的图标集
    */
   prefix?: string;
+  /**
+   * Whether the input is read-only
+   * @default false
+   */
   readonly?: boolean;
+  /**
+   * Selected icon value
+   * @default ''
+   */
   value?: string;
-  width?: string;
+  /**
+   * Width of the picker
+   * @default '100%'
+   */
+  width?: `${number}px` | `${number}%`;
 }

28-35: Consider adding prop validation.

While the defaults are sensible, consider adding runtime validation for better error handling:

-const props = withDefaults(defineProps<Props>(), {
+const props = withDefaults(defineProps<Props>({
+  pageSize: {
+    type: Number,
+    default: 36,
+    validator: (value: number) => value > 0
+  }
+}), {
   allowClear: true,
-  pageSize: 36,
   prefix: '',
   readonly: false,
   value: '',
   width: '100%',
 });

62-82: Enhance accessibility for icon picker input.

Consider adding ARIA attributes and keyboard navigation support:

 <Input
   v-model:value="currentSelect"
   :allow-clear="props.allowClear"
   :readonly="props.readonly"
   :style="{ width }"
   class="cursor-pointer"
   placeholder="点击选中图标"
+  role="combobox"
+  aria-label="Icon picker"
+  aria-expanded="false"
+  @keydown.enter="triggerPopover"
   @click="triggerPopover"
 >
playground/src/views/demos/features/icons/index.vue (1)

63-73: Consider enhancing the IconPicker implementation

While the basic implementation is good, consider these improvements for better usability:

  1. Add two-way binding to track selected icons
  2. Implement event handlers for icon selection
  3. Add loading states and error handling

Here's a suggested implementation:

-        <IconPicker width="300px" />
+        <IconPicker
+          v-model:value="selectedIconify"
+          width="300px"
+          @select="handleIconSelect"
+          @error="handleError"
+        />

-        <IconPicker prefix="svg" width="300px" />
+        <IconPicker
+          v-model:value="selectedSvg"
+          prefix="svg"
+          width="300px"
+          @select="handleSvgIconSelect"
+          @error="handleError"
+        />

Add corresponding script section:

const selectedIconify = ref('');
const selectedSvg = ref('');

const handleIconSelect = (icon: string) => {
  console.log('Selected Iconify icon:', icon);
};

const handleSvgIconSelect = (icon: string) => {
  console.log('Selected SVG icon:', icon);
};

const handleError = (error: Error) => {
  console.error('Icon picker error:', error);
};
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (2)

25-26: Translate comment to English for consistency

The comment on lines 25-26 is in Chinese. For consistency and to aid understanding for all contributors, consider translating it to English.

Apply this diff to translate the comment:

 /**
-   * 图标列表
+   * Icon list
  */

70-74: Ensure changeOpenState toggles the popover correctly

The changeOpenState method triggers a click event on refTrigger to toggle the popover's open state. This approach could lead to unexpected side effects if additional click handlers are attached to the trigger element. Consider using the popover's API to control its open state directly for clearer intent and better maintainability.

Apply this diff to use the popover's isOpen state:

 // Add a ref to the popover
+const popoverRef = ref();

 // Update the template to reference the popover
-<VbenPopover
+<VbenPopover
+  ref="popoverRef"
   :content-props="{ align: 'end', alignOffset: -11, sideOffset: 8 }"
   content-class="p-0 py-4"
 >

 // Modify the changeOpenState method
 const changeOpenState = () => {
-  if (refTrigger.value) {
-    refTrigger.value.click();
-  }
+  if (popoverRef.value) {
+    popoverRef.value.isOpen = !popoverRef.value.isOpen;
+  }
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4e88ef0 and 9fa93f3.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (12)
  • packages/@core/base/icons/src/index.ts (1 hunks)
  • packages/@core/base/icons/src/lucide.ts (2 hunks)
  • packages/@core/ui-kit/shadcn-ui/src/components/button/icon-button.vue (2 hunks)
  • packages/effects/common-ui/package.json (1 hunks)
  • packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (1 hunks)
  • packages/effects/common-ui/src/components/icon-picker/index.ts (1 hunks)
  • packages/effects/common-ui/src/components/index.ts (1 hunks)
  • packages/effects/hooks/src/index.ts (1 hunks)
  • packages/effects/hooks/src/use-pagination.ts (1 hunks)
  • playground/src/views/demos/features/icons/icon-picker.vue (1 hunks)
  • playground/src/views/demos/features/icons/icons.data.ts (1 hunks)
  • playground/src/views/demos/features/icons/index.vue (3 hunks)
✅ Files skipped from review due to trivial changes (2)
  • packages/effects/common-ui/src/components/icon-picker/index.ts
  • playground/src/views/demos/features/icons/icons.data.ts
🧰 Additional context used
🪛 GitHub Check: Lint (ubuntu-latest)
packages/effects/common-ui/package.json

[failure] 31-31:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/types'

🔇 Additional comments (13)
packages/effects/hooks/src/index.ts (1)

4-4: LGTM! Verify module existence.

The export follows the established pattern and integrates well with other hook exports.

Let's verify the module existence and potential circular dependencies:

✅ Verification successful

Module exists and no circular dependencies detected

The verification confirms:

  • use-pagination.ts module exists at the expected location
  • Only index.ts imports from use-pagination, indicating no circular dependencies
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the use-pagination module exists and check for potential circular dependencies

# Check if the module exists
fd -t f "use-pagination.ts" packages/effects/hooks/src

# Look for any circular import patterns
rg -l "from.*use-pagination" packages/effects/hooks/src

Length of output: 193

packages/@core/base/icons/src/index.ts (1)

7-12: LGTM! The export addition is well-structured.

The addition of listIcons to the exports is appropriate for implementing the icon-picker feature, as it provides the necessary functionality to enumerate available icons.

Let's verify that this export is being used by the new icon-picker component:

✅ Verification successful

The initial results show listIcons is imported from '@vben/icons' but we need to verify if this is the correct import path and matches our package name, as the export we're reviewing is in '@core/base/icons'.


We need to verify if @vben/icons is correctly aliased to our @vben-core/icons package, as there seems to be a discrepancy between the package name and the import path.


The results show there's a bridge package @vben/icons that depends on our core package @vben-core/icons. We need to verify how the listIcons export is being re-exported through this bridge package to ensure the chain of exports is correct.


We need to trace how the listIcons export flows through the bridge package's directory structure since we see it exports from './iconify/index.js' and './svg/index.js'.


The export chain is correctly implemented

The verification confirms that:

  1. The core package @vben-core/icons exports listIcons from '@iconify/vue'
  2. The bridge package @vben/icons correctly re-exports everything from '@vben-core/icons' via export * from '@vben-core/icons'
  3. The icon-picker component correctly imports listIcons from '@vben/icons'
  4. The export is being actively used in the icon-picker component
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of listIcons in the icon-picker component
# Expected: Find usage of listIcons in the new icon-picker component

# Search for icon-picker component files
echo "Searching for icon-picker component files..."
fd -t f "icon-picker" 

# Search for usage of listIcons
echo "Searching for listIcons usage..."
rg "listIcons" -A 3

Length of output: 1161


Script:

#!/bin/bash
# Let's verify the import path in the icon-picker component
# to ensure it's correctly importing from our package

# First, check the package.json to confirm our package name
echo "Checking package name in core icons package.json..."
cat packages/@core/base/icons/package.json

# Then check the actual import statement in the component
echo "\nChecking import statement context..."
rg -B 2 -A 2 "@vben/icons" playground/src/views/demos/features/icons/icon-picker.vue

Length of output: 1371


Script:

#!/bin/bash
# Let's check the package aliases in the project configuration
# to understand the mapping between @vben/icons and our package

# Check for package aliases in common configuration files
echo "Checking for package aliases..."
fd -t f "vite.config|tsconfig|package" -e json -e ts | xargs rg "@vben/icons|@vben-core/icons" -A 3

# Also check the root package.json for workspace configurations
echo "\nChecking root package.json..."
cat package.json

Length of output: 8441


Script:

#!/bin/bash
# Let's check the packages/icons directory since it appears to be the bridge package
echo "Checking the content of packages/icons/src..."
cat packages/icons/src/index.ts

# Also check if there are any re-exports of listIcons
echo "\nChecking for re-exports of listIcons in packages/icons..."
rg -B 2 -A 2 "export.*listIcons" packages/icons/

Length of output: 382


Script:

#!/bin/bash
# Let's check the iconify directory since it's being exported
echo "Checking iconify/index.js content..."
cat packages/icons/src/iconify/index.ts

# Also check the svg directory
echo "\nChecking svg/index.js content..."
cat packages/icons/src/svg/index.ts

# Check if there are any other files in iconify directory that might re-export listIcons
echo "\nListing files in iconify directory..."
ls -la packages/icons/src/iconify/

Length of output: 1679

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

3-3: LGTM! Verify the icon-picker module.

The export statement follows the established pattern and aligns with the PR objectives.

Let's verify the icon-picker module exists and exports the expected component:

✅ Verification successful

Let me check the actual exports from the index.ts file to ensure it's exporting the component correctly.


Icon picker module is properly implemented and exported

The module structure is correct with:

  • A properly implemented Vue component using Composition API
  • Correct default export in icon-picker.vue
  • Proper re-export in the barrel file index.ts
  • Complete implementation including pagination and icon selection functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the icon-picker module structure and exports

# Test 1: Check if the icon-picker directory exists
fd -t d "icon-picker$" packages/effects/common-ui/src/components

# Test 2: Check for the component file
fd -t f "icon-picker.(vue|tsx)$" packages/effects/common-ui/src/components

# Test 3: Check for the barrel file
fd -t f "index.(ts|js)$" packages/effects/common-ui/src/components/icon-picker

# Test 4: Examine the exports
rg -l "export.*IconPicker" packages/effects/common-ui/src/components/icon-picker

Length of output: 558


Script:

#!/bin/bash
# Check the actual exports in the index.ts file
cat packages/effects/common-ui/src/components/icon-picker/index.ts

# Also verify the component implementation exists in the Vue file
cat packages/effects/common-ui/src/components/icon-picker/icon-picker.vue

Length of output: 3784

packages/@core/base/icons/src/lucide.ts (1)

30-30: LGTM! New icon exports maintain consistency.

The added Grip and Package2 icons follow the established export pattern and maintain alphabetical ordering.

Let's verify these icons exist in the lucide-vue-next package:

Also applies to: 44-44

✅ Verification successful

LGTM! Icons are properly configured and available.

The verification confirms:

  • lucide-vue-next is properly listed as a dependency in the relevant packages
  • Both Grip and Package2 icons are present in the exports list at packages/@core/base/icons/src/lucide.ts
  • The icons follow the established export pattern and alphabetical ordering
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new icons exist in package.json dependencies
# and are exported by lucide-vue-next

# Test 1: Check if lucide-vue-next is listed in dependencies
echo "Checking package dependencies..."
cat package.json | jq -r '.dependencies["lucide-vue-next"] // .devDependencies["lucide-vue-next"]'

# Test 2: Search for icon imports in node_modules
# Note: This is a basic check as build artifacts aren't available in the sandbox
echo "Searching for icon exports..."
fd -t f "index.*" node_modules/lucide-vue-next -x grep -l "export.*Grip.*Package2" {} \;

Length of output: 459


Script:

#!/bin/bash
# Let's try a different approach to verify the icons
# 1. Check package.json in the base directory and parent directories
echo "Checking package.json files..."
fd -t f "package.json" . packages/@core/base -x cat {} \;

# 2. Look for actual icon imports in the codebase
echo -e "\nChecking icon usage..."
rg -l "import.*\{.*(?:Grip|Package2).*\}.*from.*lucide" --type ts

# 3. Check if these icons are used in components
echo -e "\nChecking component usage..."
rg "(?:Grip|Package2)" --type vue --type ts -l

Length of output: 52329


Script:

#!/bin/bash
# Let's verify if lucide-vue-next is properly configured and check icon usage
# 1. Check if lucide-vue-next is a direct dependency
echo "Checking lucide-vue-next dependency..."
rg '"lucide-vue-next"' --type json

# 2. Look for icon imports and usage with more flexible patterns
echo -e "\nChecking icon imports and usage..."
rg "(?:Grip|Package2)" --type ts -A 2 -B 2

# 3. Check the actual icon export file
echo -e "\nChecking icon exports..."
rg "export.*(?:Grip|Package2)" packages/@core/base/icons/src/lucide.ts

Length of output: 1121

packages/effects/hooks/src/use-pagination.ts (1)

1-2: LGTM! Imports are clean and necessary.

packages/effects/common-ui/package.json (1)

31-31: LGTM! Verify the workspace dependency.

The addition of @vben/hooks as a workspace dependency is appropriate for implementing the pagination functionality in the icon-picker component.

Let's verify the workspace package existence and its exports:

✅ Verification successful

Workspace dependency verified and correctly used

The verification confirms:

  • @vben/hooks exists as a workspace package and is used across multiple packages
  • The package exports usePagination hook from packages/effects/hooks/src/use-pagination.ts
  • The dependency is correctly referenced by other packages using workspace:*
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify @vben/hooks workspace package and its pagination hook

# Test 1: Check if the hooks package exists
fd -t f "package.json" -x grep -l "@vben/hooks"

# Test 2: Verify the pagination hook implementation
rg -t ts "export.*usePagination" packages/effects/hooks

Length of output: 496

🧰 Tools
🪛 GitHub Check: Lint (ubuntu-latest)

[failure] 31-31:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/types'

packages/@core/ui-kit/shadcn-ui/src/components/button/icon-button.vue (1)

17-17: LGTM: Props interface addition is well-structured.

The new tooltipDelayDuration prop is properly typed as an optional number and follows the existing naming conventions.

playground/src/views/demos/features/icons/icon-picker.vue (1)

23-26: LGTM!

Good use of defineOptions to prevent inheriting FormItem attributes, with clear documentation.

playground/src/views/demos/features/icons/index.vue (3)

20-21: LGTM: Clean import of IconPicker component

The import statement is properly placed and follows the component organization pattern.


50-50: LGTM: Consistent spacing

Adding margin-bottom improves the visual hierarchy and spacing between cards.


63-73: Verify IconPicker documentation

Please ensure that the IconPicker component is properly documented with its available props, events, and usage examples.

Let's check if documentation exists:

✅ Verification successful

Documentation exists in the component implementation

The IconPicker component is well-documented within its implementation. Here are the available props and events:

Props:

  • value (string, optional): Selected icon value, defaults to ''
  • pageSize (number, optional): Number of icons per page, defaults to 36
  • icons (string[], optional): Array of icon identifiers, defaults to []

Events:

  • change: Emitted when an icon is selected
  • update:value: v-model support for the selected value

The component is also documented in the general icons documentation which shows different icon usage approaches in the project. No additional documentation is needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for documentation files related to IconPicker
fd -e md -e mdx "icon-picker|icons" --exec cat {} \;

# Search for documentation in component file
rg -A 10 "^/\*\*|^///" "./playground/src/views/demos/features/icons/icon-picker.vue"

Length of output: 3728


Script:

#!/bin/bash
# Let's check the IconPicker component implementation to find props and events
rg -A 20 "IconPicker" ./playground/src/components/

# Also check if there's a dedicated component file for IconPicker
fd "icon-picker" -t f

# Let's also check for any TypeScript definitions that might document the component
fd -e ts -e d.ts "icon-picker"

Length of output: 300


Script:

#!/bin/bash
# Let's check the actual implementation of IconPicker component
cat packages/effects/common-ui/src/components/icon-picker/icon-picker.vue

# Also check for any props or types definitions in the same directory
rg -t ts "interface|type|Props" packages/effects/common-ui/src/components/icon-picker/

Length of output: 3745

packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (2)

144-144: Verify the availability of the $t function

The use of {{ $t('common.noData') }} suggests reliance on an internationalization plugin like Vue I18n. Ensure that the $t function is available in the component's context to prevent runtime errors when internationalization is not set up.

If internationalization is not intended, consider replacing it with a plain string:

- <div>{{ $t('common.noData') }}</div>
+ <div>No data available</div>

85-85: Safety check for displaying user-provided icons

When rendering icons based on user-provided input, ensure that the icon data is sanitized to prevent potential security risks like XSS attacks. Although the icons come from a predefined list, adding a safety check reinforces security.

[security]

Consider validating the icon name before rendering:

<VbenIcon :icon="currentSelect || Grip" class="size-6" />

+<!-- Add a computed property to validate the icon -->
+const safeCurrentSelect = computed(() =>
+  props.icons.includes(currentSelect.value) ? currentSelect.value : Grip
+);
+
+<!-- Update the template -->
+<VbenIcon :icon="safeCurrentSelect" class="size-6" />

packages/effects/hooks/src/use-pagination.ts Outdated Show resolved Hide resolved
packages/effects/hooks/src/use-pagination.ts Outdated Show resolved Hide resolved
playground/src/views/demos/features/icons/icon-picker.vue Outdated Show resolved Hide resolved
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: 1

🧹 Outside diff range and nitpick comments (6)
packages/effects/common-ui/package.json (1)

30-30: Consider reordering dependencies alphabetically.

To maintain consistency with the project's style, consider reordering the dependencies alphabetically.

-    "@vben/locales": "workspace:*",
     "@vben/hooks": "workspace:*",
+    "@vben/locales": "workspace:*",
🧰 Tools
🪛 GitHub Check: Lint (ubuntu-latest)

[failure] 30-30:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/locales'

playground/src/views/demos/features/icons/icon-picker.vue (3)

11-35: Add JSDoc comments for all props.

While the prefix prop is well documented, other props would benefit from similar documentation to improve component API clarity.

Add documentation for remaining props:

 export interface Props {
+  /**
+   * Whether to show clear button
+   * @default true
+   */
   allowClear?: boolean;
+  /**
+   * Number of icons to display per page
+   * @default 36
+   */
   pageSize?: number;
   /**
    * 可以通过prefix获取系统中使用的图标集
    */
   prefix?: string;
+  /**
+   * Whether the input is read-only
+   * @default false
+   */
   readonly?: boolean;
+  /**
+   * Selected icon value
+   * @default ''
+   */
   value?: string;
+  /**
+   * Width of the input
+   * @default '100%'
+   */
   width?: string;
 }

40-56: Enhance error handling and logging.

While the error handling is good, the warning message could be more informative for debugging purposes.

Consider enhancing the error handling:

 const currentList = computed(() => {
   try {
     if (props.prefix) {
       const icons = listIcons('', props.prefix);
       if (icons.length === 0) {
-        console.warn(`No icons found for prefix: ${props.prefix}`);
+        console.warn(
+          `No icons found for prefix: ${props.prefix}. ` +
+          'Please verify the prefix is correct and icons are registered.'
+        );
       }
       return icons;
     } else {
       const prefix = iconsData.prefix;
       return iconsData.icons.map((icon) => `${prefix}:${icon}`);
     }
   } catch (error) {
-    console.error('Failed to load icons:', error);
+    console.error(
+      `Failed to load icons${props.prefix ? ` for prefix ${props.prefix}` : ''}: `,
+      error instanceof Error ? error.message : error
+    );
     return [];
   }
 });

67-87: Add ARIA labels for better accessibility.

The input component should have proper ARIA labels to improve accessibility for screen readers.

Add aria-label to the input:

 <Input
   v-model:value="currentSelect"
   :allow-clear="props.allowClear"
   :readonly="props.readonly"
   :style="{ width }"
   class="cursor-pointer"
   placeholder="点击选中图标"
+  aria-label="Icon selector"
   @click="triggerPopover"
 >
packages/effects/hooks/src/use-pagination.ts (2)

17-20: Simplify the pagination logic by removing the conditional check.

The current conditional check is unnecessary because list.slice(offset, offset + pageSize) will return elements up to the end of the array even if offset + pageSize exceeds list.length. Simplifying the code improves readability without affecting functionality.

Apply this diff to simplify the code:

-  const ret =
-    offset + pageSize >= list.length
-      ? list.slice(offset)
-      : list.slice(offset, offset + pageSize);
-  return ret;
+  return list.slice(offset, offset + pageSize);

56-57: Specify explicit return type for the usePagination function.

Adding an explicit return type enhances type safety and improves code maintainability by making it clear what the function returns.

Consider defining an interface for the return type and applying it:

interface UsePaginationResult<T> {
  setCurrentPage: (page: number) => void;
  setPageSize: (pageSize: number) => void;
  getTotal: ComputedRef<number>;
  getPaginationList: ComputedRef<T[]>;
}

export function usePagination<T = any>(list: Ref<T[]>, pageSize: number): UsePaginationResult<T> {
  // existing code
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9fa93f3 and 2a631de.

📒 Files selected for processing (4)
  • packages/effects/common-ui/package.json (1 hunks)
  • packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (1 hunks)
  • packages/effects/hooks/src/use-pagination.ts (1 hunks)
  • playground/src/views/demos/features/icons/icon-picker.vue (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
🧰 Additional context used
🪛 GitHub Check: Lint (ubuntu-latest)
packages/effects/common-ui/package.json

[failure] 30-30:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/locales'

🔇 Additional comments (4)
packages/effects/common-ui/package.json (1)

30-30: LGTM: Dependency addition is appropriate.

The addition of @vben/hooks as a workspace dependency is well-aligned with the implementation of pagination functionality in the icon-picker component.

🧰 Tools
🪛 GitHub Check: Lint (ubuntu-latest)

[failure] 30-30:
Expected object keys to be in ascending order. '@vben/hooks' should be before '@vben/locales'

playground/src/views/demos/features/icons/icon-picker.vue (3)

1-10: LGTM! Imports are well-organized.

The imports are properly structured with clear separation between framework, UI components, and local data dependencies.


58-64: LGTM! Event handlers are well-implemented.

The event handlers are concise, focused, and use proper optional chaining for null safety.


4-4: Verify IconPicker component interface.

Let's verify that the IconPicker component from @vben/common-ui exists and accepts all the props we're passing to it.

Also applies to: 78-84

✅ Verification successful

Let me try a different search strategy to verify the IconPicker component.


Let me try one more search focusing on the common-ui package and its IconPicker component:


Based on the current file's usage, let me try one final search to locate the component in the monorepo:


Let me check the actual implementation of the IconPicker component now that we found its location:


IconPicker component interface verification completed successfully

The component exists in packages/effects/common-ui and accepts all the props being passed in the usage:

  • icons (array of strings) - matches the currentList computed prop being passed
  • pageSize (number) - matches the prop with default 36
  • value (string) - matches the currentSelect being passed
  • @change event - matches the handleChange handler

The implementation is complete with proper TypeScript types, event handling, and UI components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for IconPicker component definition
echo "Searching for IconPicker component definition..."
ast-grep --pattern 'export const IconPicker = defineComponent({
  $$$
})'

# Search for IconPicker props interface
echo "Searching for IconPicker props interface..."
ast-grep --pattern 'interface IconPickerProps {
  $$$
}'

Length of output: 338


Script:

#!/bin/bash
# Search for IconPicker in all files
echo "Searching for IconPicker files..."
fd IconPicker

# Search for IconPicker usage and definition patterns
echo "Searching for IconPicker patterns..."
rg -l "IconPicker" --type vue --type ts --type tsx

# Look for package dependencies
echo "Checking package dependencies..."
fd "package.json" --exec cat {} \; | grep -A 5 -B 5 "@vben/common-ui"

Length of output: 2279


Script:

#!/bin/bash
# Search specifically in the common-ui package
echo "Searching in common-ui package..."
fd . packages/common-ui -t f | grep -i iconpicker

# Search for IconPicker component content
echo "Searching IconPicker content..."
rg -t ts -t tsx "IconPicker" packages/common-ui -C 5

# Check the usage in the current file
echo "Checking current file usage..."
cat playground/src/views/demos/features/icons/icon-picker.vue

Length of output: 2351


Script:

#!/bin/bash
# Try to find the package in different possible locations
echo "Searching for IconPicker in possible monorepo locations..."
fd . -t f -e ts -e tsx -e vue | grep -i "common.*icon"

# Search for any IconPicker related files
echo "Searching for IconPicker content in all files..."
rg "IconPicker" -l --type-add 'vue:*.vue' --type-add 'tsx:*.tsx' --type vue --type tsx --type ts

# Check if it's in node_modules
echo "Checking node_modules for the package..."
fd . node_modules/@vben/common-ui -t f 2>/dev/null || echo "No node_modules found"

Length of output: 1200


Script:

#!/bin/bash
# Check the IconPicker component implementation
echo "Checking IconPicker component implementation..."
cat packages/effects/common-ui/src/components/icon-picker/icon-picker.vue

# Check the index file for exports
echo "Checking index file..."
cat packages/effects/common-ui/src/components/icon-picker/index.ts

Length of output: 4056

packages/effects/hooks/src/use-pagination.ts Show resolved Hide resolved
@anncwb anncwb merged commit 632081e into vbenjs:main Nov 9, 2024
13 checks passed
wangtong1122 pushed a commit to wangtong1122/vue-vben-admin that referenced this pull request Dec 8, 2024
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: 添加图标选择器组件
2 participants