fix: drawer component header does not take effect #4844
Conversation
|
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on simplifying the build configuration and modifying export statements. The build configuration has been streamlined to a single entry point. Export statements in various components have been adjusted, including the removal of certain exports and the addition of new ones. Additionally, the rendering logic in the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/effects/hooks/src/use-pagination.ts (2)
32-38: LGTM! Property naming improved.The renaming of computed properties from
getPaginationListandgetTotaltopaginationListandtotalrespectively follows Vue's best practices for computed property naming conventions, making the code more idiomatic.Consider adding JSDoc comments for these computed properties to maintain documentation consistency with the rest of the file:
+/** + * List of items for the current page + */ const paginationList = computed(() => { return pagination(unref(list), unref(currentPage), unref(pageSizeRef)); }); +/** + * Total number of items in the list + */ const total = computed(() => { return unref(list).length; });
56-56: LGTM! Return structure updated correctly.The return statement properly reflects the renamed properties, maintaining the hook's interface consistency.
Consider adding an explicit return type interface for better type safety and documentation:
+interface PaginationReturn { + setCurrentPage: (page: number) => void; + total: ComputedRef<number>; + setPageSize: (pageSize: number) => void; + paginationList: ComputedRef<T[]>; +} -return { setCurrentPage, total, setPageSize, paginationList }; +return { setCurrentPage, total, setPageSize, paginationList } satisfies PaginationReturn;packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (3)
41-41: Consider enhancing changeOpenState robustnessWhile the optional chaining is good, the function could be more defensive and provide feedback when the click fails.
Consider this enhancement:
function changeOpenState() { - refTrigger.value?.click?.(); + if (!refTrigger.value?.click) { + console.warn('Icon picker trigger element not found'); + return; + } + refTrigger.value.click(); }Also applies to: 78-80
53-54: Enhance pagination accessibilityWhile the pagination logic works, consider these accessibility improvements:
- Add ARIA labels for navigation
- Ensure keyboard navigation works for all pagination controls
Consider these enhancements:
<Pagination v-slot="{ page }" :items-per-page="36" :sibling-count="1" :total="total" show-edges size="small" + aria-label="Icon list navigation" @update:page="handlePageChange" > <PaginationList v-slot="{ items }" class="flex w-full items-center gap-1" > - <PaginationFirst class="size-5" /> - <PaginationPrev class="size-5" /> + <PaginationFirst class="size-5" aria-label="First page" /> + <PaginationPrev class="size-5" aria-label="Previous page" />Also applies to: 112-156
87-87: Extract magic numbers and improve maintainabilityThe template changes look good, but there are some improvements possible:
- The hardcoded pagination size (36) should be referenced from props
- The empty state handling is well implemented
Consider this enhancement:
<Pagination v-slot="{ page }" - :items-per-page="36" + :items-per-page="pageSize" :sibling-count="1"Also applies to: 91-91, 157-164
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
182-187: Consider enhancing the accessibility fallback content.Good addition of accessibility support using
VisuallyHidden! However, the emptySheetTitleandSheetDescriptioncomponents could be more meaningful for screen readers.Consider this improvement:
<template v-else> <VisuallyHidden> - <SheetTitle /> - <SheetDescription /> + <SheetTitle>{{ title || $t('drawer.defaultTitle') }}</SheetTitle> + <SheetDescription>{{ description || $t('drawer.defaultDescription') }}</SheetDescription> </VisuallyHidden> </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/@core/base/icons/build.config.ts(1 hunks)packages/@core/base/icons/src/index.ts(0 hunks)packages/@core/base/icons/src/lucide.ts(0 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue(2 hunks)packages/effects/common-ui/src/components/icon-picker/icon-picker.vue(5 hunks)packages/effects/hooks/src/use-pagination.ts(2 hunks)packages/icons/src/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/@core/base/icons/src/index.ts
- packages/@core/base/icons/src/lucide.ts
🔇 Additional comments (7)
packages/icons/src/index.ts (1)
2-2: Verify the existence of EmptyIcon component.
The export statement looks correct, but let's verify the existence of the EmptyIcon component.
✅ Verification successful
EmptyIcon component exists and is actively used in the codebase
The EmptyIcon component is:
- Properly implemented as an SVG icon in
packages/icons/src/icons/empty-icon.vue - Used in multiple components:
packages/effects/plugins/src/vxe-table/use-vxe-grid.vuepackages/effects/common-ui/src/components/icon-picker/icon-picker.vue
The export statement is valid and the component is being utilized appropriately across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of empty-icon.vue and its usage
# Test 1: Check if empty-icon.vue exists
fd --type f "empty-icon.vue$" packages/icons/src/icons
# Test 2: Check if EmptyIcon is used in the codebase
rg "EmptyIcon" --type vue --type ts
Length of output: 160
Script:
#!/bin/bash
# Let's verify the EmptyIcon usage with a more reliable approach
# Check if empty-icon.vue exists and show its content
cat packages/icons/src/icons/empty-icon.vue
# Search for EmptyIcon usage without file type restrictions
rg "EmptyIcon"
Length of output: 1442
packages/@core/base/icons/build.config.ts (2)
Line range hint 1-8: Question: Is this change related to the drawer header fix?
This build configuration change appears unrelated to the PR's objective of fixing the drawer component header visibility issue (#4843). Could you please clarify why this change is included in this PR?
#!/bin/bash
# Check if there are any connections between this change and drawer components
echo "Checking for relationships between icons and drawer components:"
rg -l "drawer.*icon|icon.*drawer"6-6: Verify build configuration changes impact.
The simplified build configuration might affect how icons are bundled and imported. Please ensure:
- Icon components are still properly processed
- TypeScript declarations are generated correctly
- Tree-shaking still works as expected
✅ Verification successful
Build configuration change is safe and follows package patterns
The simplified build configuration for the icons package is valid because:
- The entry point
src/index.tsexists and exports all necessary icon-related functionality - The configuration matches the pattern used in other packages (e.g., shared package)
- The build setup includes essential flags (
clean: true,declaration: true) consistent with other packages - The single entry point is sufficient as the index file properly re-exports all required components and utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for icon imports and verify the new entry point exists
echo "Verifying src/index existence and content:"
find . -name "src" -type d -exec find {} -name "index.ts" \;
# Check for potential icon usage patterns that might be affected
echo "Checking icon usage patterns:"
rg -l "import.*from.*@core/base/icons"
Length of output: 459
Script:
#!/bin/bash
# Check if src/index.ts exists in the icons package
echo "Checking for src/index.ts in icons package:"
ls -la packages/@core/base/icons/src/index.ts 2>/dev/null || echo "File not found"
# Check the content of the build config file
echo -e "\nChecking build config content:"
cat packages/@core/base/icons/build.config.ts
# Look for any icon-related imports across the codebase
echo -e "\nChecking icon imports patterns:"
rg -A 2 "from ['\"](\.\.\/|@core\/base\/icons)" --type ts --type vue
# Check if there are other build configs for comparison
echo -e "\nChecking other build configs in the codebase:"
fd "build.config.ts" packages/@core
Length of output: 1411
Script:
#!/bin/bash
# Check other build configs for comparison
echo "Checking content of other build configs:"
for config in packages/@core/base/shared/build.config.ts packages/@core/ui-kit/layout-ui/build.config.ts; do
echo -e "\n=== $config ==="
cat "$config"
done
# Check the content of icons/src/index.ts
echo -e "\nChecking icons index.ts content:"
cat packages/@core/base/icons/src/index.ts
# Look for icon imports with fixed regex
echo -e "\nChecking icon import patterns:"
rg "from ['\"]@core/base/icons" --type ts --type vue -A 2
Length of output: 1832
packages/effects/hooks/src/use-pagination.ts (1)
Line range hint 32-56: Verify all usages of the pagination hook.
Let's ensure all components using this hook have been updated to use the new property names.
✅ Verification successful
No breaking changes found in usePagination hook usage
The verification shows that there's only one component using the usePagination hook - icon-picker.vue. This component is already using the correct property names from the hook's return value:
const { paginationList, total, setCurrentPage } = usePagination(The search results confirm that there are no instances of old property names (getPaginationList, getTotal) in the codebase, and the component is correctly using the new property names (paginationList, total, setCurrentPage). All other matches in the search results are unrelated to this hook's usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that import and use the pagination hook
# to verify they've been updated to use the new property names
# Search for files importing the hook
echo "Files importing usePagination:"
rg -l "import.*usePagination.*from.*effects/hooks"
# Search for old property usage that might need updating
echo "\nChecking for old property names that need updating:"
rg "getPaginationList|getTotal" --type vue --type ts
Length of output: 323
Script:
#!/bin/bash
# Let's try a different approach to find usages
# First, find all possible imports of usePagination
echo "Files importing usePagination (broader search):"
rg -l "usePagination"
# Then look for actual usage patterns of the hook's return values
echo -e "\nChecking usage patterns:"
rg -A 2 "const.*=.*usePagination\("
# Also search for specific property access
echo -e "\nChecking property access:"
rg "paginationList|setCurrentPage|setPageSize|total"
Length of output: 6610
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (2)
2-5: LGTM! Clean import restructuring
The import changes improve code organization by:
- Using the composition API's
useTemplateRef - Replacing deprecated icons with new ones
Line range hint 1-166: Verify relationship with PR objectives
This icon picker component changes appear unrelated to the PR's main objective of fixing the drawer component header visibility issue (#4843).
Let's verify if this file is actually needed for the drawer header fix:
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
135-135: Excellent fix for header visibility issue!
Changing from v-show to v-if ensures complete removal of the header from the DOM when showHeader is false, effectively resolving the issue where the header couldn't be fully hidden. This is a more robust solution as it:
- Prevents any CSS-related visibility issues
- Improves performance for elements that don't frequently toggle
- Ensures proper cleanup of header-related DOM elements
Description
fixed #4843
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Release Notes
EmptyIconcomponent for easier access in the public API.SheetHeadercomponent for better accessibility.icon-pickercomponent for improved clarity and functionality.Package2export, cleaning up unused icons.