Remove unnecessary scrollbars in skeleton tab #8148
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SegmentListItem
participant ContextMenu
participant MeshInfoItem
participant ScrollableVirtualizedTree
User->>SegmentListItem: Click on segment
SegmentListItem->>ContextMenu: Show context menu
ContextMenu->>User: Display options (load, edit, delete)
User->>ContextMenu: Select load option
ContextMenu->>MeshInfoItem: Check mesh status
MeshInfoItem-->>ContextMenu: Return mesh info
ContextMenu->>User: Show toast notification
User->>ScrollableVirtualizedTree: View updated tree
Assessment against linked issues
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 (5)
frontend/stylesheets/trace_view/_right_menu.less (2)
Line range hint
89-98: Consider adjusting tree-list container styles to better handle overflow.The current
#tree-liststyles might contribute to the scrollbar issues. Consider:
- Adding
overflow: hiddento prevent unintended scrollbars- Using
min-heightinstead ofheight: inheritfor better flexibility#tree-list { - height: inherit; + min-height: 0; display: flex; flex-direction: column; + overflow: hidden; .ant-space-compact { // Spacing between skeleton tab buttons and name input margin: 0 0 10px 0; } }
142-159: Additional styles needed to fully address scrollbar issues.While the button styling improvements are valuable, they don't fully address the scrollbar issues mentioned in PR #8058. Consider adding styles to:
- Handle long tree names with word-wrapping
- Control list container overflow
Add these styles to handle the remaining issues:
// Add to handle long tree names .ant-tree-title { word-break: break-word; white-space: normal; } // Add to control list container overflow .ant-tree-list { overflow-x: hidden; overflow-y: auto; }frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (2)
Line range hint
307-313: Consider UI/UX improvements for mesh info displayThe "No mesh loaded" message improves user feedback, but consider these enhancements:
- Move inline styles to a CSS class for better maintainability
- Add a smooth transition for action button visibility changes
- <div className="deemphasized italic" style={{ marginLeft: 8 }}> + <div className="mesh-info-message deemphasized italic"> No mesh loaded. Use right-click to add one. </div>Add to CSS:
.mesh-info-message { margin-left: 8px; } .mesh-info-actions { transition: visibility 0.2s ease; }
Line range hint
566-571: Optimize layout structure to prevent overflowThe nested flex containers and inline styles could contribute to width issues. Consider:
- Consolidating layout styles into CSS classes
- Using CSS Grid for better space distribution
- Adding
text-overflow: ellipsisfor long segment names- <List.Item - style={{ - padding: "2px 5px", - }} + <List.Item + className="segment-list-item-container" className={`segment-list-item ${isHoveredSegmentId ? "is-hovered-segment" : ""}`} onMouseEnter={() => { setHoveredSegmentId(segment.id); }} - <div style={{ display: "inline-flex", alignItems: "center" }}> + <div className="segment-list-item-header">Add to CSS:
.segment-list-item-container { padding: 2px 5px; } .segment-list-item-header { display: inline-flex; align-items: center; min-width: 0; /* Enable text truncation */ }Also applies to: 577-580
frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (1)
874-926: LGTM: Improved button layout with flex-wrap support.The new button layout with flex-wrap handles narrow screens gracefully. The firstButton/lastButton classes and overflow handling are implemented correctly.
Consider adding aria-label attributes to improve accessibility:
-<div +<div + aria-label="Tree management actions" style={{ display: "inline-flex", flexWrap: "wrap" }} className="compact-buttons" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_renderers.tsx(2 hunks)frontend/stylesheets/trace_view/_right_menu.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_renderers.tsx
🔇 Additional comments (5)
frontend/stylesheets/trace_view/_right_menu.less (1)
142-159: Verify button group behavior with different viewport sizes.
The .compact-buttons implementation looks good for creating connected button groups, but there are some considerations:
- Negative margins could cause layout issues in narrow viewports
- The styles assume buttons are direct children
Consider these improvements:
- Add a media query for narrow viewports
- Use child selectors for better specificity
.compact-buttons {
- button {
+ > button {
border-radius: 0px;
margin-inline: -1px;
margin-top: -1px;
}
- button.firstButton {
+ > button.firstButton {
border-top-left-radius: 4px;
border-bottom-left-radius: 4px;
}
- button.lastButton {
+ > button.lastButton {
border-top-right-radius: 4px;
border-bottom-right-radius: 4px;
}
+
+ @media (max-width: 480px) {
+ > button {
+ margin-inline: 0;
+ border-radius: 4px;
+ }
+ }
}Let's verify the button layout behavior:
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx (2)
Line range hint 486-507: Well-implemented error handling and user feedback
The implementation provides excellent user feedback with:
- Clear confirmation dialogs for destructive actions
- Actionable toast notifications
- Consistent error handling patterns
63-64: Verify alignment changes against PR objectives
The style changes to ColoredDotIcon modify its vertical alignment, but these changes are in the segments tab while the PR objectives specifically mention fixing scrollbar issues in the skeleton tab. Please verify if these changes are intentional or if they were meant for a different PR.
frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx (2)
856-856: LGTM: Overflow hidden fixes the multiple scrollbars issue.
The addition of overflow: "hidden" to the main container effectively prevents the appearance of unnecessary scrollbars while still allowing child elements to scroll independently. This change directly addresses the PR objective.
949-949: LGTM: Consistent overflow handling.
The addition of overflow: "clip" to the sort button maintains consistency with other buttons and prevents potential text overflow issues.
There was a problem hiding this comment.
Thanks a lot for this PR. The multi scrollbars were pretty annoying 🙏
in general the user experience should be nice for reasonable resizing of the tab. of course when very narrow, the tab is not really usable
Yeah, UX feels better now. Thanks a lot :D
Please find my few comments below (only small things). Afterwards, it should be ready to merge 🚀
frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx
Outdated
Show resolved
Hide resolved
| margin-inline: -1px; | ||
| margin-top: -1px; |
There was a problem hiding this comment.
The result looks great, thanks 🙏
But you please explain why a -1px is needed here? Does this move the inner buttons one px to the left and one 1 🤔?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx(3 hunks)frontend/stylesheets/trace_view/_right_menu.less(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx
- frontend/stylesheets/trace_view/_right_menu.less
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~42-~42: You might be missing the article “the” here.
Context: ...55) - Removed unnesessary scrollbars in skeleton tab that occured especially after resiz...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
42-42: Enhance the changelog entry with more details and fix grammar.The entry could be more descriptive and needs minor grammatical improvements.
-- Removed unnecessary scrollbars in skeleton tab that occurred especially after resizing. [#8148](https://github.com/scalableminds/webknossos/pull/8148) +- Removed unnecessary scrollbars in the skeleton tab that occurred especially after resizing, improving the display of long tree names with line breaks instead of horizontal scrolling. [#8148](https://github.com/scalableminds/webknossos/pull/8148)🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...cessary scrollbars in skeleton tab that occurred especially after resizing. [#8148](http...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...cessary scrollbars in skeleton tab that occurred especially after resizing. [#8148](http...
(AI_HYDRA_LEO_MISSING_COMMA)
* Wrap antd tree to enable scrolling while dragging * add explanation to biome ignore comment * add changelog entry * fix typing * extract magic numbers into constants * apply feedback - send datasource id in correct format to backend - fix dataset renaming in dataset settings - fix typo in filename --------- Co-authored-by: Michael Büßemeyer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (1)
13-15: Consider making scroll behavior configurable.The component could benefit from optional props to customize scroll behavior, such as scroll speeds and areas, instead of using hard-coded constants.
function ScrollableVirtualizedTree<T extends BasicDataNode>( - props: TreeProps<T> & { ref: React.RefObject<RcTree> }, + props: TreeProps<T> & { + ref: React.RefObject<RcTree>; + minScrollSpeed?: number; + maxScrollSpeed?: number; + scrollAreaRatio?: number; + minScrollAreaHeight?: number; + }, ) { + const { + minScrollSpeed = MIN_SCROLL_SPEED, + maxScrollSpeed = MAX_SCROLL_SPEED, + scrollAreaRatio = SCROLL_AREA_RATIO, + minScrollAreaHeight = MIN_SCROLL_AREA_HEIGHT, + ...treeProps + } = props;frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx (2)
Line range hint
19-27: Good enhancement of SegmentHierarchyGroup with BasicDataNode.The extension of BasicDataNode provides the necessary properties for proper tree node rendering and layout management in antd's Tree component. This change supports the PR's goal of improving scrollbar behavior.
Consider documenting the key BasicDataNode properties that are inherited, as they are crucial for the Tree component's proper functioning. This will help future maintainers understand the relationship between these types and the antd Tree component.
29-34: Good enhancement of SegmentHierarchyLeaf with BasicDataNode.The type now properly extends both BasicDataNode and Segment, ensuring all necessary properties are available for tree node rendering.
Consider removing explicit declarations of 'key' and 'title' if they're already provided by BasicDataNode to avoid property duplication:
export type SegmentHierarchyLeaf = BasicDataNode & Segment & { type: "segment"; - key: string; - title: string; };frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
280-285: Consider adding error boundaries for AutoSizerWhile the wrapper implementation looks good, consider adding error boundaries to gracefully handle cases where AutoSizer fails to calculate dimensions correctly.
+const ErrorBoundary = ({ children }: { children: React.ReactNode }) => { + return ( + <React.Suspense fallback={<div>Loading...</div>}> + {children} + </React.Suspense> + ); +}; <AutoSizer> {({ height, width }) => ( + <ErrorBoundary> <div ref={wrapperRef} style={{ height, width, }} > <ScrollableVirtualizedTree {...props} /> </div> + </ErrorBoundary> )} </AutoSizer>frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
Line range hint
1907-1937: LGTM: Proper implementation of virtualized treeThe ScrollableVirtualizedTree implementation correctly maintains all existing functionality while adding virtualization support. The explicit height prop enables proper virtualization, and the style adjustments ensure correct rendering.
Consider extracting the tree configuration into a separate constant or helper function to improve maintainability, as there are many props being passed. For example:
const getTreeConfig = (props: TreeConfigProps) => ({ allowDrop: props.allowDrop, onDrop: props.onDrop, onSelect: props.onSelect, className: "segments-tree", blockNode: true, height: props.height, // ... other props });Then use it as:
<ScrollableVirtualizedTree<SegmentHierarchyNode> {...getTreeConfig({ height, allowDrop: this.allowDrop, onDrop: this.onDrop, // ... other props })} />
Line range hint
1-2000: Consider breaking down this large component for better maintainabilityThe SegmentsView component is quite large (~2000 lines) and handles many responsibilities. While it works correctly, it could benefit from some architectural improvements:
Consider the following improvements:
- Extract the mesh-related functionality into a separate component or custom hook (e.g.,
useMeshManagement)- Move the context menu logic into a separate component (e.g.,
SegmentContextMenu)- Create a separate component for the details panel
- Extract common utility functions into a separate helper file
This would make the code more maintainable, testable, and easier to understand. Example structure:
// hooks/useMeshManagement.ts export const useMeshManagement = () => { // Extract mesh-related state and methods return { handleLoadMeshesAdHoc, handleLoadMeshesFromFile, // ... other mesh-related methods }; }; // components/SegmentContextMenu.tsx export const SegmentContextMenu = () => { // Extract context menu rendering logic }; // components/SegmentDetails.tsx export const SegmentDetails = () => { // Extract details panel logic };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(5 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx
- frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[typographical] ~42-~42: Usually, there’s no comma before “when”.
Context: ...task annotations of teams you are not in, when accessing directly via URI. [#8155](htt...
(IF_NO_COMMA)
🔇 Additional comments (17)
frontend/javascripts/oxalis/view/right-border-tabs/scrollable_virtualized_tree.tsx (2)
1-11: LGTM! Well-structured imports and reasonable constant values.
The imports are appropriate, and the scroll-related constants provide a good balance between usability and performance.
16-55: Optimize getBoundingClientRect() calls and verify scroll behavior.
The implementation looks solid, but there are two considerations:
- Multiple getBoundingClientRect() calls in the throttled function could be optimized
- The scroll behavior should be verified with different tree sizes and viewport dimensions
Consider caching the bounding rectangles:
throttle((info: { event: React.DragEvent<HTMLDivElement> }) => {
const target = info.event.target as HTMLElement;
if (!target || !wrapperRef.current) {
return;
}
- const { bottom: currentBottom, top: currentTop } = target.getBoundingClientRect();
- const { bottom: boxBottom, top: boxTop } = wrapperRef.current.getBoundingClientRect();
+ const targetRect = target.getBoundingClientRect();
+ const wrapperRect = wrapperRef.current.getBoundingClientRect();
+ const { bottom: currentBottom, top: currentTop } = targetRect;
+ const { bottom: boxBottom, top: boxTop } = wrapperRect;✅ Verification successful
Let me try another verification approach with different commands:
The previous commands had issues with file type recognition. These modified commands should help us verify the scroll implementation and potential conflicts.
Let me try one final verification approach without file type flags:
The scroll implementation is well-isolated and correctly implemented
The verification shows that:
- The
scrollTopmanipulation is unique to this component and doesn't conflict with other implementations - The
onDragOverhandler is properly scoped to theScrollableVirtualizedTreecomponent - The component is used in both
segments_view.tsxandtree_hierarchy_view.tsx, but the scroll behavior is encapsulated within the component itself
While the suggested optimization for getBoundingClientRect() calls could improve performance slightly, the current implementation is correct and doesn't have any functional issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar scroll implementations and potential issues
# Test 1: Look for other scroll implementations that might conflict
rg -A 5 "scrollTop" --type typescript
# Test 2: Check for other drag-and-drop handlers that might interfere
ast-grep --pattern 'onDragOver($$$)'
# Test 3: Look for related test files
fd "scrollable.*test" --type file
Length of output: 403
Script:
#!/bin/bash
# Look for scroll-related implementations with proper file type
rg -t ts -t tsx -A 5 "scrollTop"
# Look for drag handlers in TypeScript/React files
rg -t ts -t tsx -A 5 "onDragOver"
# Search for test files with proper extension
fd ".*test\.(ts|tsx)$" --type file
Length of output: 267
Script:
#!/bin/bash
# Search for scroll-related implementations
rg "scrollTop" --type-add 'script:*.{ts,tsx}' -t script -A 5
# Look for drag handlers
rg "onDragOver" --type-add 'script:*.{ts,tsx}' -t script -A 5
# Look for similar virtualized tree components
rg "VirtualizedTree" --type-add 'script:*.{ts,tsx}' -t script -A 5
Length of output: 6917
CHANGELOG.unreleased.md (2)
35-35: LGTM: Clear and well-formatted changelog entry.
The entry clearly describes the fix for scrolling functionality during drag operations.
43-43: LGTM: Changelog entry accurately reflects PR objectives.
The entry correctly describes the fix for unnecessary scrollbars in the skeleton tab, aligning with the PR objectives and maintaining good changelog formatting.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view_helper.tsx (2)
15-15: LGTM: BasicDataNode import is appropriate.
The import of BasicDataNode from antd's tree component is necessary for the type extensions and aligns with the goal of improving the tree component's layout.
Line range hint 15-34: Verify integration with ScrollableVirtualizedTree component.
These type changes appear to support the new ScrollableVirtualizedTree component mentioned in the AI summary. Let's verify the integration:
✅ Verification successful
Integration with ScrollableVirtualizedTree is properly implemented and type-safe
The verification shows that:
- The types are correctly integrated with
ScrollableVirtualizedTreeinsegments_view.tsx - The component is properly imported and used with the correct generic type
<SegmentHierarchyNode> - The type hierarchy (Group/Leaf) is consistently used across the codebase
- All necessary props (allowDrop, onDrop, onSelect) are properly passed to the tree component
The type changes successfully support the tree rendering and scrollbar functionality as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of these types with ScrollableVirtualizedTree
# and ensure there are no type mismatches.
echo "Checking for ScrollableVirtualizedTree usage with these types..."
rg -l "ScrollableVirtualizedTree.*SegmentHierarchy(Group|Leaf)" --type typescript
echo "Checking for any type-related TODO/FIXME comments..."
rg "TODO|FIXME|XXX" -C2 --type typescript | rg -i "type|scroll|tree"
Length of output: 478
Script:
#!/bin/bash
# Let's try a different approach without typescript file type specification
# First, let's find the ScrollableVirtualizedTree component usage
echo "Checking for ScrollableVirtualizedTree usage..."
rg "ScrollableVirtualizedTree.*SegmentHierarchy"
# Let's also check where these types are being used
echo -e "\nChecking SegmentHierarchyGroup and SegmentHierarchyLeaf usage..."
rg "SegmentHierarchyGroup|SegmentHierarchyLeaf"
# Let's look for the ScrollableVirtualizedTree component definition
echo -e "\nLocating ScrollableVirtualizedTree component..."
fd ScrollableVirtualizedTree
Length of output: 1361
Script:
#!/bin/bash
# Let's check the actual usage context in segments_view.tsx
echo "Checking ScrollableVirtualizedTree usage context..."
rg -B5 -A5 "ScrollableVirtualizedTree.*SegmentHierarchy" frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
# Let's also check for the SegmentHierarchyNode type usage
echo -e "\nChecking SegmentHierarchyNode type usage..."
rg "SegmentHierarchyNode" -B2 -A2
# Let's verify the imports in segments_view.tsx
echo -e "\nChecking imports in segments_view.tsx..."
rg "^import.*ScrollableVirtualizedTree" frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
Length of output: 14415
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (4)
2-2: LGTM: Import changes align with scrollbar optimization goals
The introduction of ScrollableVirtualizedTree and reorganization of imports support the PR's objective of improving scrollbar behavior.
Also applies to: 28-29, 43-43
65-65: LGTM: Added wrapper ref for improved layout control
The addition of wrapperRef provides better control over the container's layout, which is essential for managing scrollbar behavior.
Line range hint 286-321: LGTM: ScrollableVirtualizedTree implementation addresses scrollbar issues
The implementation properly handles virtualization and layout constraints, which should resolve the excessive scrollbar issues mentioned in #8058.
Line range hint 319-319: Verify the impact of negative margin style
The marginLeft: -14 style might cause layout issues in certain scenarios, such as when the tab width is very narrow.
✅ Verification successful
Let me analyze the search results for negative margins in the codebase. I can see that negative margins are used in several places, and specifically in the tree components:
- In
tree_hierarchy_view.tsx(the file in question) withmarginLeft: -14 - In
segments_tab/segments_view.tsxwithmarginLeft: -26which has a comment explaining it's used to "hide switcherIcon for root group"
Let me verify if there are any specific Tree component styles or if this is part of a design pattern.
Let me try a different approach to understand the Tree component usage and its styling context.
Based on the search results, I can see that the negative margin of -14px is used in the Tree component within tree_hierarchy_view.tsx. Looking at the changelog, this file is part of a larger refactoring where the skeleton tab component was replaced with antd's <Tree /> component (PR #7819). Let me check the specific component implementation to understand the context better.
Let me check if this is a standard pattern for antd Tree components by looking at other Tree implementations in the codebase.
Based on the available context, I can now make a determination about the negative margin:
The negative margin style is safe to use
The -14px left margin is part of the antd Tree component styling in the skeleton tab view, which was part of a planned refactoring (as seen in CHANGELOG.released.md PR #7819). The margin appears to be intentional to align the tree with other UI elements, and is used alongside other Tree props like showLine, blockNode, and autoExpandParent which are standard antd Tree configurations. The component is contained within a tab panel which provides proper padding/spacing constraints, so there's no risk of layout issues even with narrow widths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of negative margins in the codebase to understand the pattern
rg -B 2 -A 2 "margin.*-\d+"
Length of output: 11733
Script:
#!/bin/bash
# Search for Tree component styles and usage
rg -B 2 -A 2 "Tree.*style"
Length of output: 28
Script:
#!/bin/bash
# Look for the Tree component implementation and its usage
rg -B 3 -A 3 "<Tree"
Length of output: 38367
Script:
#!/bin/bash
# Look for the Tree component implementation in tree_hierarchy_view.tsx
rg -B 5 -A 5 "marginLeft: -14" frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx
Length of output: 661
Script:
#!/bin/bash
# Look for other Tree components with showLine prop to understand the styling pattern
ast-grep --pattern 'Tree $$$showLine$$$'
Length of output: 42
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (3)
54-55: LGTM: Import paths are correctly updated.
The import paths for HideTreeEdgesIcon and ColoredDotIcon components are properly structured using relative paths.
93-93: LGTM: Word-break style effectively handles long tree names.
The addition of wordBreak: "break-word" style directly addresses the PR objective by ensuring long tree names wrap properly, eliminating the need for horizontal scrollbars.
Also applies to: 217-217
219-220: LGTM: Icon rendering is simplified and properly spaced.
The FolderOutlined icon rendering is improved with:
- Removal of unnecessary span wrapper
- Consistent margin spacing using the
icon-margin-rightclass
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (3)
89-89: LGTM: Import paths are correctly updated.
The import paths have been properly updated to reflect the new directory structure.
Also applies to: 93-94
368-368: LGTM: Generator return type is more precise.
The return type change from void to undefined is correct as it more accurately represents the generator's behavior when it doesn't yield a final value.
855-855: LGTM: Layout improvements effectively address scrollbar issues.
The changes successfully address the scrollbar issues mentioned in PR #8148:
- Setting
overflow: "hidden"on the main container prevents unnecessary scrollbars - Using
Space.Compactwith proper button organization improves layout structure - Added appropriate className attributes for styling consistency
These changes align well with the PR objectives to enhance the user experience by removing unnecessary scrollbars.
Also applies to: 873-923, 946-946
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
138-138: LGTM: Clean import of ScrollableVirtualizedTree
The import is properly organized with other imports and uses a clear relative path.
| return ( | ||
| <div ref={wrapperRef}> | ||
| <AntdTree {...props} onDragOver={onDragOver} /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| export default ScrollableVirtualizedTree; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add style props to control wrapper dimensions and overflow.
Given that this PR aims to fix scrollbar issues, the wrapper div should have configurable styles to control its dimensions and overflow behavior.
+type WrapperStyle = {
+ height?: string | number;
+ maxHeight?: string | number;
+ width?: string | number;
+ maxWidth?: string | number;
+};
function ScrollableVirtualizedTree<T extends BasicDataNode>(
props: TreeProps<T> & {
ref: React.RefObject<RcTree>;
+ wrapperStyle?: WrapperStyle;
},
) {
+ const { wrapperStyle, ...treeProps } = props;
return (
- <div ref={wrapperRef}>
- <AntdTree {...props} onDragOver={onDragOver} />
+ <div
+ ref={wrapperRef}
+ style={{
+ overflow: "hidden",
+ ...wrapperStyle,
+ }}
+ >
+ <AntdTree {...treeProps} onDragOver={onDragOver} />
</div>
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div ref={wrapperRef}> | |
| <AntdTree {...props} onDragOver={onDragOver} /> | |
| </div> | |
| ); | |
| } | |
| export default ScrollableVirtualizedTree; | |
| type WrapperStyle = { | |
| height?: string | number; | |
| maxHeight?: string | number; | |
| width?: string | number; | |
| maxWidth?: string | number; | |
| }; | |
| function ScrollableVirtualizedTree<T extends BasicDataNode>( | |
| props: TreeProps<T> & { | |
| ref: React.RefObject<RcTree>; | |
| wrapperStyle?: WrapperStyle; | |
| }, | |
| ) { | |
| const { wrapperStyle, ...treeProps } = props; | |
| return ( | |
| <div | |
| ref={wrapperRef} | |
| style={{ | |
| overflow: "hidden", | |
| ...wrapperStyle, | |
| }} | |
| > | |
| <AntdTree {...treeProps} onDragOver={onDragOver} /> | |
| </div> | |
| ); | |
| } | |
| export default ScrollableVirtualizedTree; |
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
ScrollableVirtualizedTreecomponent for improved tree view performance with drag-and-drop capabilities.Improvements
Bug Fixes