feat: add click-to-click event support to the WorkenchProject and WorkenchQuickNav components #4831
Conversation
|
WalkthroughThe changes involve modifications to several Changes
Assessment against linked 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: 8
🧹 Outside diff range and nitpick comments (6)
packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue (1)
24-25: Add type safety to event emit declaration.While the implementation works, we should leverage TypeScript to ensure type safety for the emitted event payload.
-defineEmits(['click']); +const emit = defineEmits<{ + (e: 'click', item: WorkbenchQuickNavItem): void; +}>();packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue (3)
24-25: Consider adding TypeScript type definitions for the click event.While the event declaration works, adding TypeScript types would improve type safety and developer experience.
-defineEmits(['click']); +const emit = defineEmits<{ + (e: 'click', item: WorkbenchProjectItem): void; +}>();
Line range hint
33-49: Fix inconsistent click behavior.Currently, there's a UX inconsistency:
- The entire card shows a pointer cursor (cursor:pointer) suggesting it's clickable
- However, only the icon actually emits the click event
- This creates a misleading user experience
Move the click handler to the parent div for consistent behavior:
<div :class="{ 'border-r-0': index % 3 === 2, 'border-b-0': index < 3, 'pb-4': index > 2, }" class="border-border group w-full cursor-pointer border-b border-r border-t p-4 transition-all hover:shadow-xl md:w-1/2 lg:w-1/3" + @click="$emit('click', item)" > <div class="flex items-center"> <VbenIcon :color="item.color" :icon="item.icon" class="size-8 transition-all duration-300 group-hover:scale-110" - @click="$emit('click', item)" />
Line range hint
1-25: Consider adding built-in navigation support.Based on issue #4825, this component should support direct navigation via
router.pushor URL opening. Consider adding these features:
- Add optional props for navigation:
interface Props { items: WorkbenchProjectItem[]; title: string; + navigationMode?: 'router' | 'url' | 'none'; }
- Add navigation utilities:
const router = useRouter(); const { openWindow } = useGlobSetting(); const handleNavigation = (item: WorkbenchProjectItem) => { emit('click', item); if (props.navigationMode === 'router' && item.route) { router.push(item.route); } else if (props.navigationMode === 'url' && item.url) { openWindow(item.url); } };This would provide built-in navigation while maintaining the flexibility of custom click handlers.
apps/web-antd/src/views/dashboard/workspace/index.vue (2)
207-237: Consider refactoring the navigation logic for better maintainability.The current switch statement implementation has several potential improvements:
- Hard-coded routes could be moved to a centralized route configuration
- Switch statement could be replaced with a more maintainable object map
Consider refactoring to:
const ROUTE_MAP = { '仪表盘': '/dashboard', '图表': '/analytics', '权限管理': '/demos/access/page-control', '系统管理': '/demos/features/login-expired', '组件': '/demos/features/icons', '首页': '/', } as const; function navTo(nav: WorkbenchQuickNavItem) { const route = ROUTE_MAP[nav.title as keyof typeof ROUTE_MAP]; if (route) { router.push(route); } }
253-257: Consider enhancing accessibility and user feedback.Since these components are now interactive:
- Add visual indicators (cursor: pointer, hover states) to show they're clickable
- Consider adding loading states during navigation
- Add ARIA attributes for better accessibility
Example styling improvements:
<style scoped> .clickable { cursor: pointer; transition: opacity 0.2s; } .clickable:hover { opacity: 0.8; } </style>And update components to use these styles along with ARIA attributes:
<WorkbenchProject :items="projectItems" title="项目" + class="clickable" + role="button" + aria-label="Click to open project" @click="toProject" />Also applies to: 265-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/web-antd/src/views/dashboard/workspace/index.vue(4 hunks)apps/web-ele/src/views/dashboard/workspace/index.vue(4 hunks)apps/web-naive/src/views/dashboard/workspace/index.vue(4 hunks)packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue(2 hunks)packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue(2 hunks)playground/src/views/dashboard/workspace/index.vue(4 hunks)
🔇 Additional comments (12)
packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue (1)
Line range hint 24-42: Implementation successfully meets PR objectives.
The changes effectively implement click event support as requested in issue #4825, allowing parent components to handle navigation through router.push or URL opening. The code is clean and follows Vue 3 best practices.
Let's verify the parent component implementations:
✅ Verification successful
Implementation successfully meets PR objectives and is properly used by parent components.
The verification confirms that:
- The click event is properly implemented in
workbench-quick-nav.vue - Parent components across different UI frameworks (Naive, Antd, Element) correctly:
- Import and use the WorkbenchQuickNav component
- Handle the click event with a
navTofunction - Implement proper navigation logic using Vue Router's
router.push - Pass the required items prop with proper typing (WorkbenchQuickNavItem[])
The implementation aligns perfectly with issue #4825's requirements and follows Vue 3 best practices for component communication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify parent components are properly handling the click event
# Expected: Find implementations of click handlers in workspace components
# Search for WorkbenchQuickNav usage with click handlers
rg -A 5 '<WorkbenchQuickNav.*@click'
Length of output: 38
Script:
#!/bin/bash
# Let's try a broader search to find parent components using WorkbenchQuickNav
# First, find the component usage
rg -l "WorkbenchQuickNav"
# Then search for the file containing this component to understand its structure
fd "workbench-quick-nav.vue"
Length of output: 540
Script:
#!/bin/bash
# Let's examine how WorkbenchQuickNav is used in the parent components
rg -A 10 "WorkbenchQuickNav" "playground/src/views/dashboard/workspace/index.vue" "apps/web-naive/src/views/dashboard/workspace/index.vue" "apps/web-antd/src/views/dashboard/workspace/index.vue" "apps/web-ele/src/views/dashboard/workspace/index.vue"
Length of output: 17333
apps/web-antd/src/views/dashboard/workspace/index.vue (2)
10-10: LGTM: Import statements are correctly placed and necessary.
The added imports for useRouter and openWindow are appropriate for implementing the new navigation functionality.
Also applies to: 22-22
253-257: LGTM: Click event handlers are properly implemented.
The click event handlers are correctly bound to the components, fulfilling the PR objective of adding click event support to WorkbenchProject and WorkbenchQuickNav components.
Also applies to: 265-265
apps/web-ele/src/views/dashboard/workspace/index.vue (3)
10-10: LGTM: Import statements are properly organized.
The new imports are correctly placed and necessary for the added navigation functionality.
Also applies to: 22-22
253-257: LGTM: Event handlers are properly implemented.
The click event handlers are correctly bound to the components and follow Vue 3 conventions.
Also applies to: 265-265
253-257: Verify click event implementation in child components.
Please ensure that the WorkbenchProject and WorkbenchQuickNav components are properly implementing the click event emission as mentioned in the PR objectives.
Also applies to: 265-265
✅ Verification successful
Click events are properly implemented in both components
Both WorkbenchProject and WorkbenchQuickNav components correctly:
- Define click events using
defineEmits(['click']) - Emit click events with item data using
$emit('click', item)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify click event implementation in components
# Check WorkbenchProject component
echo "Checking WorkbenchProject component..."
ast-grep --pattern 'defineEmits([["\'"]click["\'"]]) $$$' packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
# Check WorkbenchQuickNav component
echo "Checking WorkbenchQuickNav component..."
ast-grep --pattern 'defineEmits([["\'"]click["\'"]]) $$$' packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
Length of output: 181
Script:
#!/bin/bash
# Let's try a different approach to verify the event emission
# Search for click event emission in WorkbenchProject
echo "=== WorkbenchProject click event usage ==="
rg -A 2 "emit.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
# Search for defineEmits in WorkbenchProject
echo -e "\n=== WorkbenchProject defineEmits ==="
rg "defineEmits" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
# Search for click event emission in WorkbenchQuickNav
echo -e "\n=== WorkbenchQuickNav click event usage ==="
rg -A 2 "emit.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
# Search for defineEmits in WorkbenchQuickNav
echo -e "\n=== WorkbenchQuickNav defineEmits ==="
rg "defineEmits" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
# Let's also check if these files exist
echo -e "\n=== Checking if files exist ==="
fd "workbench-(project|quick-nav).vue" packages/effects/common-ui/src/ui/dashboard/
Length of output: 1338
apps/web-naive/src/views/dashboard/workspace/index.vue (3)
10-10: LGTM: Import statements are properly organized.
The new imports for useRouter and openWindow are correctly placed and necessary for the navigation functionality.
Also applies to: 22-22
253-257: LGTM: Click event handlers are properly implemented.
The click event handlers are correctly bound to their respective navigation functions, fulfilling the requirements from issue #4825.
Also applies to: 265-265
253-257: Verify click event implementation in child components.
Let's ensure that the WorkbenchProject and WorkbenchQuickNav components properly define and emit the click events we're listening to.
Also applies to: 265-265
✅ Verification successful
Let me try a different approach to verify the event implementations.
Click events are properly implemented in both components
Both WorkbenchProject and WorkbenchQuickNav components correctly define and emit the 'click' event:
- Both components declare
defineEmits(['click']) - WorkbenchProject emits click with item data:
@click="$emit('click', item)" - WorkbenchQuickNav emits click with item data:
@click="$emit('click', item)"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify click event definitions in components
# Check WorkbenchProject component
echo "Checking WorkbenchProject component for click event definition..."
ast-grep --pattern 'defineEmits([$$$ "click" $$$])' packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
# Check WorkbenchQuickNav component
echo "Checking WorkbenchQuickNav component for click event definition..."
ast-grep --pattern 'defineEmits([$$$ "click" $$$])' packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
Length of output: 545
Script:
#!/bin/bash
# Search for click event emits in the components using ripgrep for broader context
echo "Checking WorkbenchProject component..."
rg -A 5 -B 5 "emit.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
echo -e "\nChecking WorkbenchQuickNav component..."
rg -A 5 -B 5 "emit.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
# Also check for defineEmits declarations with different pattern
echo -e "\nChecking defineEmits declarations..."
rg -A 2 "defineEmits|emits:" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
Length of output: 2107
playground/src/views/dashboard/workspace/index.vue (3)
10-10: LGTM: Import statements are correctly placed and necessary.
The new imports support the click event handling functionality as per PR objectives.
Also applies to: 22-22
253-257: LGTM: Click event handlers are properly implemented.
The template changes successfully implement the click event support as specified in the PR objectives. The event handlers are correctly bound to their respective navigation functions.
Also applies to: 265-265
253-257: Verify click event definitions in component files.
Let's ensure that the click events are properly defined in the respective components.
Also applies to: 265-265
✅ Verification successful
Click events are properly defined in both components
Both WorkbenchProject and WorkbenchQuickNav components correctly define the click event using defineEmits(['click']), which matches the usage in the parent component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify click event definitions in components
# Check WorkbenchProject component
echo "Checking WorkbenchProject component..."
rg -A 5 "defineEmits.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
# Check WorkbenchQuickNav component
echo "Checking WorkbenchQuickNav component..."
rg -A 5 "defineEmits.*click" packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
Length of output: 561
packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
Show resolved
Hide resolved
anncwb
changed the title
…nd WorkbenchQuickNav components
…tem for enhanced navigation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
packages/effects/common-ui/src/ui/dashboard/typing.ts (1)
18-18: LGTM! Consider adding JSDoc comments.The addition of the optional
urlproperty toWorkbenchProjectItemis well-structured and maintains backward compatibility.Consider adding JSDoc comments to document the expected URL format and usage:
+ /** URL for navigation. Can be an external link or internal route path */ url?: string;apps/web-antd/src/views/dashboard/workspace/index.vue (2)
112-112: Move TODO comment out of the data structure.The TODO comment about URL adjustment should be moved to a separate comment block or documentation rather than being embedded in the data structure.
- url: '/demos/features/login-expired', // 这里的 URL 是示例，实际项目中需要根据实际情况进行调整 + url: '/demos/features/login-expired',
221-233: Enhance error handling and input validation.While the navigation logic is sound, consider these improvements:
- Add input validation for the nav parameter
- Provide more specific error messages
- Consider adding a loading state during navigation
function navTo(nav: WorkbenchProjectItem | WorkbenchQuickNavItem) { + if (!nav?.url) { + console.warn(`Navigation item ${nav?.title || 'unknown'} has no URL`); + return; + } + if (nav.url?.startsWith('http')) { openWindow(nav.url); return; } if (nav.url?.startsWith('/')) { + // Consider adding loading state here router.push(nav.url).catch((error) => { - console.error('Navigation failed:', error); + console.error(`Navigation to ${nav.url} failed:`, error); + // Consider showing user feedback here }); } else { console.warn(`Unknown URL for navigation item: ${nav.title} -> ${nav.url}`); } }apps/web-ele/src/views/dashboard/workspace/index.vue (1)
Line range hint
28-125: LGTM: URL properties are well-structured with good documentation.The mix of internal routes and external URLs is well-documented with helpful comments. Consider adding URL validation at the type level for additional safety.
Consider adding a type union for URL validation:
type NavigationUrl = { url: `/${''}${string}` | `http${'s' | ''}://${string}`; }; type WorkbenchProjectItem = { // ... existing properties } & NavigationUrl; type WorkbenchQuickNavItem = { // ... existing properties } & NavigationUrl;apps/web-naive/src/views/dashboard/workspace/index.vue (2)
28-30: Consider using i18n for comments.The mixed language comments (Chinese and English) might affect maintainability. Consider using i18n for better internationalization support.
Line range hint
31-84: Consider moving URLs to configuration.The hardcoded URLs should be moved to a configuration file for better maintainability and easier updates. This is especially important for external URLs that might change over time.
Example configuration structure:
// config/project-items.ts export const PROJECT_URLS = { GITHUB: 'https://github.com', VUE: 'https://vuejs.org', HTML5: 'https://developer.mozilla.org/zh-CN/docs/Web/HTML', // ... other URLs } as const;playground/src/views/dashboard/workspace/index.vue (3)
28-30: Translate comments to English for better maintainability.Consider translating the Chinese comments to English to ensure all team members can understand the code documentation.
-// 这是一个示例数据，实际项目中需要根据实际情况进行调整 -// url 也可以是内部路由，在 navTo 方法中识别处理，进行内部跳转 -// 例如：url: /dashboard/workspace +// This is sample data that should be adjusted according to actual project needs +// URLs can also be internal routes, which will be handled by the navTo method for internal navigation +// Example: url: /dashboard/workspace
Line range hint
31-84: Consider improvements to URL handling in project items.Several suggestions for the project items configuration:
- Consider removing locale-specific URLs (e.g.,
/zh-CN/) to support internationalization.- Add URL format validation to ensure all URLs are properly formatted.
- Consider using environment variables for base URLs to make the configuration more maintainable.
Consider creating a type validator for URLs:
function isValidUrl(url: string): boolean { try { new URL(url); return true; } catch { return url.startsWith('/'); } } interface ValidatedWorkbenchProjectItem extends WorkbenchProjectItem { url: string; } function validateProjectItem(item: WorkbenchProjectItem): ValidatedWorkbenchProjectItem { if (!item.url || !isValidUrl(item.url)) { throw new Error(`Invalid URL in project item: ${item.title}`); } return item as ValidatedWorkbenchProjectItem; } const projectItems = [ // ... your items ... ].map(validateProjectItem);
219-233: Enhance navigation error handling and user experience.The navigation logic is good but could be improved in several ways:
- Add user-friendly error handling
- Add loading state management
- Add type discrimination between project and quick nav items
Consider this enhanced implementation:
type NavItem = WorkbenchProjectItem | WorkbenchQuickNavItem; function isProjectItem(item: NavItem): item is WorkbenchProjectItem { return 'group' in item; } const loading = ref(false); async function navTo(nav: NavItem) { loading.value = true; try { if (nav.url?.startsWith('http')) { openWindow(nav.url); return; } if (nav.url?.startsWith('/')) { await router.push(nav.url); // Optional: Different handling based on item type if (isProjectItem(nav)) { // Handle project navigation } else { // Handle quick nav navigation } } else { console.warn(`Invalid URL for ${isProjectItem(nav) ? 'project' : 'quick nav'} item: ${nav.title}`); } } catch (error) { console.error('Navigation failed:', error); // Add user-friendly error notification here } finally { loading.value = false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/web-antd/src/views/dashboard/workspace/index.vue(10 hunks)apps/web-ele/src/views/dashboard/workspace/index.vue(10 hunks)apps/web-naive/src/views/dashboard/workspace/index.vue(10 hunks)packages/effects/common-ui/src/ui/dashboard/typing.ts(2 hunks)packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue(2 hunks)packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue(2 hunks)playground/src/views/dashboard/workspace/index.vue(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/effects/common-ui/src/ui/dashboard/workbench/workbench-project.vue
- packages/effects/common-ui/src/ui/dashboard/workbench/workbench-quick-nav.vue
🔇 Additional comments (14)
packages/effects/common-ui/src/ui/dashboard/typing.ts (1)
39-39: LGTM! Verify URL handling implementation.
The addition of the optional url property to WorkbenchQuickNavItem successfully addresses issue #4825, enabling both internal routing and external URL navigation.
Let's verify the implementation of URL handling across components:
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the URL handling implementation.
URL handling implementation verified and properly implemented
The codebase shows consistent and secure implementation of URL handling across components:
- External URLs are handled via
openWindowutility with proper security features (noopener, noreferrer) - Internal routing uses
router.pushwith error handling for URLs starting with '/' - Implementation is consistent across all web app variants (web-naive, web-antd, web-ele)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of URL handling in components using these interfaces
# Check for router.push usage with WorkbenchQuickNavItem
echo "Checking router.push implementation for WorkbenchQuickNavItem:"
rg -A 5 "WorkbenchQuickNavItem.*url"
# Check for openWindow usage with external URLs
echo "Checking external URL handling:"
rg -A 5 "openWindow.*url"
# Verify click event handlers in components
echo "Checking click event implementation:"
ast-grep --pattern 'defineEmits\(\[.*"click".*\]\)'
Length of output: 3718
apps/web-antd/src/views/dashboard/workspace/index.vue (5)
10-10: LGTM: Import statements are correctly added.
The new imports for useRouter and openWindow are necessary for implementing the navigation functionality.
Also applies to: 22-22
28-30: LGTM: Well-documented example data structure.
The comments effectively explain that this is example data and how URLs can be used for both internal and external navigation.
39-39: Security: Sanitize URLs before use.
The URLs in projectItems should be sanitized before use to prevent XSS attacks.
Also applies to: 48-48, 57-57, 66-66, 75-75, 84-84
88-124: LGTM: Well-structured navigation items.
The quickNavItems array is well-organized with clear routing paths for internal navigation.
249-249: LGTM: Click events are properly implemented.
The click events are correctly bound to the navTo function in both WorkbenchProject and WorkbenchQuickNav components, fulfilling the PR objectives.
Also applies to: 257-257
apps/web-ele/src/views/dashboard/workspace/index.vue (2)
10-10: LGTM: Import statements are correctly placed and necessary.
The added imports support the new navigation functionality.
Also applies to: 22-22
249-249: LGTM: Click event handlers are properly implemented.
The click event handlers are correctly bound to the navTo function, fulfilling the PR objectives for click-to-click event support.
Also applies to: 257-257
apps/web-naive/src/views/dashboard/workspace/index.vue (3)
10-10: LGTM: Import statements are correctly implemented.
The new imports for useRouter and openWindow are appropriately placed and necessary for the navigation functionality.
Also applies to: 22-22
249-249: LGTM: Event handlers are correctly implemented.
The click event handlers are properly bound to the navTo function following Vue 3 conventions.
Also applies to: 257-257
88-125: Add route validation.
The internal routes should be validated against the actual router configuration to prevent navigation to non-existent routes.
Let's verify the routes exist in the router configuration:
playground/src/views/dashboard/workspace/index.vue (3)
10-10: LGTM: Import statements are properly organized.
The new imports for useRouter and openWindow are correctly placed and necessary for the navigation functionality.
Also applies to: 22-22
88-124: Improve route management in quick nav items.
The previous review comment about using a route map constant is still valid. This would help:
- Centralize route management
- Prevent typos in route paths
- Make route updates easier to maintain
- Support type checking for routes
Additionally, translate the Chinese comment to English:
-// 同样，这里的 url 也可以使用以 http 开头的外部链接
+// Similarly, URLs here can also be external links starting with http249-249: LGTM: Click event handlers are properly implemented.
The click event handlers are correctly added to both components and properly bound to the navTo function.
Also applies to: 257-257
Description
New Feature
Added support for click events in the WorkbenchProject and WorkbenchQuickNav components, allowing custom behavior to be executed on click.
Defined the click event using defineEmits(['click']), enabling developers to pass in a function that will execute when the component is clicked.
Code Changes
Added defineEmits(['click']) to both WorkbenchProject and WorkbenchQuickNav components to define the click event.
Developers can now listen for the click event in the parent component and pass in a custom function to respond to click actions.
Closes #4825
Type of change
Please delete options that are not relevant.
Checklist
pnpm run docs:devcommand.pnpm run test:unit.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes
Documentation