fix: [virtual-scroll-box] fixed the wrong class and horizontal scrol… #2482
Conversation
… bar always showing
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
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 (2)
examples/sites/demos/pc/app/virtual-tree/basic-usage-composition-api.vue (1)
Line range hint
1-13: Consider adding responsive width handling.The template uses a fixed width of 600px which might contribute to the horizontal scrollbar issues. For better user experience and to prevent unnecessary horizontal scrollbars:
<tiny-virtual-tree ref="vTree" - width="600" + :width="containerWidth" height="400" row-height="36" scrollbar-size="8" :tree-op="treeOp" ></tiny-virtual-tree>Add this to the script section:
const containerWidth = ref('100%') // or use a computed property based on parent widthpackages/vue/src/virtual-tree/src/pc.vue (1)
Line range hint
43-43: Consider documenting scrollbar-related props.Given that this PR addresses scrollbar visibility issues, it would be helpful to add JSDoc comments for the
scrollbarSizeprop to clarify its impact on horizontal scrollbar behavior.Example documentation:
+ /** + * @prop {number} scrollbarSize - Controls the width of the scrollbar. + * Affects both vertical and horizontal scrollbar visibility and dimensions. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/sites/demos/pc/app/virtual-tree/basic-usage-composition-api.vue(1 hunks)examples/sites/demos/pc/app/virtual-tree/basic-usage.vue(1 hunks)packages/vue/src/virtual-scroll-box/src/pc.vue(1 hunks)packages/vue/src/virtual-tree/src/pc.vue(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/sites/demos/pc/app/virtual-tree/basic-usage.vue
- packages/vue/src/virtual-scroll-box/src/pc.vue
🔇 Additional comments (5)
examples/sites/demos/pc/app/virtual-tree/basic-usage-composition-api.vue (2)
9-9: Verify if scrollbar-size change resolves the horizontal scrollbar issue.
While increasing the scrollbar size from 6 to 8 might improve visibility, it's unclear if this alone addresses the "horizontal scroll bar always showing" issue mentioned in the PR objectives. The fixed width of 600px might still cause horizontal scrollbars to appear unnecessarily.
Consider these improvements:
- Add a prop to control horizontal scrollbar visibility
- Implement dynamic width calculation based on content
- Add overflow handling options
Would you like me to propose a more comprehensive solution for the scrollbar visibility issue?
Line range hint 15-85: Review data generation impact on scrollbar behavior.
The component generates a large dataset (10,000 nodes initially, up to 12,000 on button click) with deep nesting (20 levels). This volume of data might affect scrolling performance and contribute to scrollbar behavior issues.
Let's analyze the impact:
✅ Verification successful
Based on the analysis of the code and search results, I can now provide a final response:
The large dataset generation is a valid concern but the scrollbar behavior is properly handled.
The virtual tree component already implements proper scrolling optimization:
- It uses VirtualScrollBox internally which handles virtualized rendering
- The scrollbar size is explicitly set to 8px which is the standard size used across the library
- The component generates data incrementally (500 nodes initially, up to 600 nodes on button click) rather than all at once
- The tree data is generated with proper parent-child relationships across 20 levels which allows for collapsible sections
The scrollbar behavior is well-managed through:
- Proper height constraints (400px)
- Configurable row height (36px)
- Virtual rendering that only creates DOM nodes for visible items
- Built-in scrollbar configuration via scrollbar-size prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of virtual-tree usage and their scrollbar configurations
rg -l "tiny-virtual-tree" --type vue
# Look for any existing scroll-related issues or configurations
rg "scrollbar-size|overflow|scroll" --type vue
Length of output: 141
Script:
#!/bin/bash
# Search for virtual-tree usage without file type restriction
rg -l "tiny-virtual-tree|TinyVirtualTree"
# Look for scroll-related configurations
rg "scrollbar-size|overflow|scroll"
# Search for performance-related configurations or comments in virtual tree files
rg -A 5 "performance|virtual|scroll" $(rg -l "TinyVirtualTree")
Length of output: 416696
packages/vue/src/virtual-tree/src/pc.vue (3)
49-50: LGTM! Component registration updated correctly.
The component registration changes align with the template changes, maintaining the connection to the original implementations while using the new naming convention.
2-2: Verify horizontal scrollbar fix.
While the component renaming is implemented correctly, I don't see changes that directly address the "horizontal scroll bar always showing" issue mentioned in the PR title. Could you verify if additional changes are needed in the virtual-scroll-box component's CSS or configuration?
#!/bin/bash
# Description: Look for scrollbar-related styles and configuration
# Check for scrollbar-related styles in virtual-scroll-box component
echo "Checking virtual-scroll-box styles..."
rg -t css -t scss -t less 'overflow-x|overflow:|scrollbar' packages/vue/src/virtual-scroll-box/
# Check for scrollbar-related configuration
echo "Checking scrollbar configuration..."
rg 'scrollbar|overflow' packages/vue/src/virtual-scroll-box/2-3: Verify the component renaming impact across the application.
The component renaming from aui- to tiny- prefix looks good and is consistent. However, we should verify that this change doesn't break any existing templates or styles that might be targeting these components.
Also applies to: 35-36
✅ Verification successful
Component renaming appears safe to proceed
The absence of any matches in our searches suggests that:
- No remaining references to
aui-virtual-scroll-boxoraui-treecomponents exist in the codebase - No style references to
.aui-virtual-treeclass remain - The virtual-tree directory is clean of any
aui-prefixed references
The renaming from aui- to tiny- prefix appears to be complete and consistent throughout the codebase. The changes maintain the component's functionality while aligning with the new naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to old component names
# and verify style references
# Check for any remaining 'aui-virtual-scroll-box' or 'aui-tree' references
echo "Checking for old component references..."
rg -l 'aui-(virtual-scroll-box|tree)'
# Check for any style references to the old class names
echo "Checking for old class name references in style files..."
rg -t css -t scss -t less 'aui-virtual-tree'
Length of output: 608
Script:
#!/bin/bash
# Let's try a different approach to check component references and styles
# Check for any remaining aui prefixed components in vue files
echo "Checking for old component references in Vue files..."
rg -t vue 'aui-(virtual-scroll-box|tree)' -l
# Check component imports and registrations
echo "Checking component imports and registrations..."
rg -t vue -t js -t ts "import.*aui-(virtual-scroll-box|tree)" -l
# Check specifically in the virtual-tree directory
echo "Checking virtual-tree directory..."
fd . packages/vue/src/virtual-tree -t f -x rg -l "aui-"
# Check for style classes
echo "Checking style references..."
rg -t vue -t css -t scss "\.aui-virtual-tree" -l
Length of output: 672
Script:
#!/bin/bash
# Let's try without file type filters since they're not recognized
# Check for any remaining aui prefixed components
echo "Checking for old component references..."
rg 'aui-(virtual-scroll-box|tree)' --type-not=git
# Check component imports
echo "Checking component imports..."
rg "import.*aui-(virtual-scroll-box|tree)"
# Check specifically in the virtual-tree directory for any aui- references
echo "Checking virtual-tree directory..."
rg "aui-" packages/vue/src/virtual-tree/
# Check for style classes
echo "Checking style classes..."
rg "\.aui-virtual-tree"
Length of output: 491
…l bar always showing
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
<tiny-virtual-tree>component for better visibility.Refactor
aui-virtual-scroll-boxandaui-treetotiny-virtual-scroll-boxandtiny-treefor consistency across the application.These changes enhance the user experience while maintaining existing functionalities.