fix: limit the drag range of tabs, fixed #4640 #4659
Conversation
|
WalkthroughThe changes in this pull request focus on enhancing the tab components within the UI kit. Modifications include the introduction of a new class Changes
Possibly related PRs
Suggested reviewers
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 (3)
packages/@core/ui-kit/tabs-ui/src/use-tabs-drag.ts (1)
84-91: Enhance robustness of tab dragging logicThe implementation successfully limits dragging between fixed and non-fixed tabs, aligning with the PR objectives. However, consider the following improvements for increased robustness:
- Add a fallback for cases where the
affix-tabclass might be missing:- const isCurrentAffix = evt.dragged.classList.contains('affix-tab'); - const isRelatedAffix = evt.related.classList.contains('affix-tab'); + const isCurrentAffix = evt.dragged.classList.contains('affix-tab') || false; + const isRelatedAffix = evt.related.classList.contains('affix-tab') || false;
- Consider adding logging for unexpected scenarios:
if (parent?.classList.contains('draggable') && props.draggable) { const isCurrentAffix = evt.dragged.classList.contains('affix-tab') || false; const isRelatedAffix = evt.related.classList.contains('affix-tab') || false; + if (isCurrentAffix !== isRelatedAffix) { + console.debug('Prevented dragging between fixed and non-fixed tabs'); + } // 不允许在固定的tab和非固定的tab之间互相拖拽 return isCurrentAffix === isRelatedAffix; } else { + console.debug('Drag prevented: parent not draggable or dragging disabled'); return false; }These changes will make the code more resilient to edge cases and provide better debugging information.
packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue (1)
79-79: LGTM: Correctly applies the 'affix-tab' classThe addition of the 'affix-tab' class to the tab item div is well-implemented. It correctly uses the
tab.affixTabproperty to conditionally apply the class, which allows for distinct styling of fixed tabs.Consider adding a CSS rule for the 'affix-tab' class in your stylesheet to visually distinguish fixed tabs. For example:
.affix-tab { /* Add styles to visually indicate a fixed tab */ font-weight: bold; /* or */ border-bottom: 2px solid #your-color; }This will enhance the user experience by making it clear which tabs are fixed.
packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue (1)
72-78: LGTM! Consider a minor readability improvement.The addition of the 'affix-tab' class based on the
tab.affixTabproperty aligns well with the PR objective of limiting the drag range for tabs. The existing logic for 'is-active' and 'draggable' classes remains intact, ensuring consistent behavior.For improved readability, consider destructuring
tab.affixTabat the beginning of the template. This would make the code more concise and easier to understand:<template> <div v-for="(tab, i) in tabsView" :key="tab.key" ref="tabRef" :class="[ { 'is-active': tab.key === active, draggable: !affixTab, 'affix-tab': affixTab, }, ]" ... > <VbenContextMenu :handler-data="tab" :menus="contextMenus" :modal="false" item-class="pr-6" > <!-- Destructure affixTab here --> <template #default="{ affixTab }"> <!-- Rest of the template --> </template> </VbenContextMenu> </div> </template>This change would make the class binding more concise and easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue (1 hunks)
- packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue (1 hunks)
- packages/@core/ui-kit/tabs-ui/src/use-tabs-drag.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/@core/ui-kit/tabs-ui/src/use-tabs-drag.ts (1)
Line range hint
1-141: Overall assessment: Implementation aligns with PR objectivesThe changes successfully implement the feature to limit the drag range of tabs, preventing dragging between fixed and non-fixed tabs. The implementation is efficient and aligns well with the existing codebase structure.
A few minor improvements have been suggested to enhance robustness and debugging capabilities. These suggestions do not impact the core functionality but will make the code more resilient to edge cases.
Consider running additional tests to ensure the drag-and-drop functionality works as expected across various scenarios, including edge cases where the
affix-tabclass might be missing or incorrectly set.To verify the implementation, please run the following script:
This script will help ensure that the
affix-tabclass is used consistently across the project and that related drag-and-drop functionality is properly implemented. It will also identify potential test files where you might want to add or update tests for this new feature.✅ Verification successful
Follow-Up: Corrected Shell Scripts for Verification
The previous shell scripts encountered an issue with unrecognized file types for
vue. Please run the following corrected scripts to continue the verification process:These corrected scripts explicitly target
.vueand.tsfiles, ensuring that Ripgrep searches the appropriate file types for the specified patterns. Please execute these scripts and provide the output to proceed with the verification.
Verification Complete: Changes Confirmed
The search results confirm that the
'affix-tab'class and drag-and-drop related code are correctly implemented in the specified files. Additionally, relevant test files are present to ensure functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of 'affix-tab' class and drag-and-drop related code # Test 1: Check for 'affix-tab' class usage echo "Checking for 'affix-tab' class usage:" rg --type vue --type typescript "affix-tab" # Test 2: Verify drag-and-drop related code echo "\nChecking for drag-and-drop related code:" rg --type vue --type typescript -e "draggable" -e "onMove" -e "sortable" # Test 3: Look for potential test files echo "\nLooking for potential test files:" fd -e spec.ts -e test.tsLength of output: 2528
Script:
#!/bin/bash # Corrected shell scripts to verify 'affix-tab' usage and drag-and-drop related code # Test 1: Check for 'affix-tab' class usage in Vue and TypeScript files echo "Checking for 'affix-tab' class usage:" rg -g '*.vue' -g '*.ts' "affix-tab" # Test 2: Verify drag-and-drop related code in Vue and TypeScript files echo "\nChecking for drag-and-drop related code:" rg -g '*.vue' -g '*.ts' -e "draggable" -e "onMove" -e "sortable" # Test 3: Look for potential test files echo "\nLooking for potential test files:" fd -e spec.ts -e test.tsLength of output: 7748
packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue (2)
79-79: LGTM: Correctly implements theaffixTabpropertyThe addition of the
affixTabproperty to each tab object in thetabsViewcomputed property is well-implemented. It correctly uses themeta.affixTabvalue to determine if a tab is fixed, which aligns with the PR objective of distinguishing between fixed and non-fixed tabs.
Line range hint
1-158: Summary: Successfully implemented tab affixing featureThe changes in this file effectively implement the feature to distinguish between fixed and non-fixed tabs. The modifications are minimal, focused, and align well with the PR objectives. Both the script and template changes work together cohesively to add the necessary functionality.
Key points:
- The
tabsViewcomputed property now includes anaffixTabproperty for each tab.- The template correctly applies the 'affix-tab' class based on this new property.
These changes provide a solid foundation for limiting the drag range of tabs as per the PR description. The implementation follows Vue.js best practices and integrates smoothly with the existing codebase.
packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue (1)
72-78: Changes align well with PR objectives and maintain component integrity.The addition of the 'affix-tab' class enhances the styling capabilities for affixed tabs, supporting the PR objective of limiting the drag range for tabs. This change provides a visual distinction for affixed tabs without altering the existing functionality of tab interaction and rendering.
The modifications are focused and minimal, reducing the risk of unintended side effects. The changes maintain the integrity of the component while introducing the new feature as intended.
To ensure that these changes are properly integrated and don't conflict with other parts of the codebase, please run the following verification script:
This script will help identify any potential conflicts or areas that might need additional updates to fully implement the new feature.
✅ Verification successful
Changes align well with PR objectives and maintain component integrity.
The addition of the 'affix-tab' class enhances the styling capabilities for affixed tabs, supporting the PR objective of limiting the drag range for tabs. This change provides a visual distinction for affixed tabs without altering the existing functionality of tab interaction and rendering.
The verification scripts confirmed that there are no conflicting hardcoded 'affix-tab' classes outside the targeted component and that the
affixTabproperty is used consistently across the codebase. Additionally, drag-and-drop functionalities related to tabs are appropriately managed without introducing any conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of 'affix-tab' class and related functionality # Test 1: Check for any hardcoded 'affix-tab' classes that might conflict echo "Checking for hardcoded 'affix-tab' classes:" rg --type vue --type css 'class="[^"]*affix-tab' -g '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue' # Test 2: Verify that 'affixTab' property is consistently used echo "Verifying 'affixTab' property usage:" rg --type vue --type ts 'affixTab' -g '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue' # Test 3: Check for any drag-and-drop related functions that might need updating echo "Checking drag-and-drop related functions:" rg --type vue --type ts 'drag|drop' -g '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue'Length of output: 708
Script:
#!/bin/bash # Description: Verify the usage of 'affix-tab' class and related functionality # Test 1: Check for any hardcoded 'affix-tab' classes that might conflict echo "Checking for hardcoded 'affix-tab' classes:" rg 'class="[^"]*affix-tab' -g "*.vue" -g "*.css" --glob '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue' # Test 2: Verify that 'affixTab' property is consistently used echo "Verifying 'affixTab' property usage:" rg 'affixTab' -g "*.vue" -g "*.ts" --glob '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue' # Test 3: Check for any drag-and-drop related functions that might need updating echo "Checking drag-and-drop related functions:" rg 'drag|drop' -g "*.vue" -g "*.ts" --glob '!packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue'Length of output: 11377
anncwb
changed the title
Description
限制标签栏的拖拽范围，只能在同类型的标签之间拖拽：固定标签不能拖拽到非固定标签中，非固定标签也不能拖拽到固定标签中。
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
New Features
affix-tabclass.Bug Fixes