feat: Drag and drop: show collections in collapsed state#3136
feat: Drag and drop: show collections in collapsed state#3136matheusandre1 wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPropagates collapse state and group-child detection into node containers, conditions rendering of the child-count badge to only when collapsed, adds optional collection icon for nodes with group children, and adjusts FloatingCircle styles to size to content. Changes
Sequence Diagram(s)sequenceDiagram
participant Element
participant CustomNode
participant CustomNodeContainer
participant FloatingCircle
Element->>CustomNode: render with element data
CustomNode->>CustomNodeContainer: pass props (childCount, isCollapsed, hasGroupChildren)
alt Dragged overlay
CustomNode->>CustomNodeContainer: render overlay (receives isCollapsed, hasGroupChildren)
end
CustomNodeContainer->>FloatingCircle: render if isCollapsed && childCount > 0
alt hasGroupChildren
FloatingCircle->>FloatingCircle: add .step-icon-collection class and render Layers icon
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx (1)
138-139: Avoid callinggetAllNodeChildren()twice.Lines 138 and 139 each invoke
element.getAllNodeChildren(). Reuse a single local to avoid duplicate traversal on every render.♻️ Proposed refactor
- const childCount = element.getAllNodeChildren().length; - const hasGroupChildren = element.getAllNodeChildren().some((node) => node.getData()?.vizNode?.data?.isGroup); + const nodeChildren = element.getAllNodeChildren(); + const childCount = nodeChildren.length; + const hasGroupChildren = nodeChildren.some((node) => node.getData()?.vizNode?.data?.isGroup);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx` around lines 138 - 139, The code calls element.getAllNodeChildren() twice when computing childCount and hasGroupChildren; store the result in a local const (e.g., const children = element.getAllNodeChildren()) and use children.length for childCount and children.some(...) for hasGroupChildren so you avoid repeating traversal in CustomNode (references: element.getAllNodeChildren, childCount, hasGroupChildren).packages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsx (1)
232-252: Consider tightening thesvgassertion.
container.querySelector('svg')will match any SVG in the tree. Scoping the query to the.step-icon__collectionelement (e.g.,container.querySelector('.step-icon__collection svg')) would more precisely assert that theLayersicon is rendered inside the collection badge and guard against false positives if other SVGs are introduced later.♻️ Proposed refactor
- expect(container.querySelector('.step-icon__collection')).toBeInTheDocument(); - // Carbon icon is rendered inside PF Icon - expect(container.querySelector('svg')).toBeInTheDocument(); + const collectionBadge = container.querySelector('.step-icon__collection'); + expect(collectionBadge).toBeInTheDocument(); + // Carbon Layers icon is rendered inside the PF Icon within the collection badge + expect(collectionBadge?.querySelector('svg')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsx` around lines 232 - 252, The test "should render Layers icon when hasGroupChildren is true" in CustomNodeContainer.test.tsx should scope the SVG assertion to the collection badge: replace the broad container.querySelector('svg') check with a scoped query such as container.querySelector('.step-icon__collection svg') so the expectation ensures the Layers icon is rendered inside the .step-icon__collection element rendered by the CustomNodeContainer component when hasGroupChildren is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx`:
- Around line 138-139: The code calls element.getAllNodeChildren() twice when
computing childCount and hasGroupChildren; store the result in a local const
(e.g., const children = element.getAllNodeChildren()) and use children.length
for childCount and children.some(...) for hasGroupChildren so you avoid
repeating traversal in CustomNode (references: element.getAllNodeChildren,
childCount, hasGroupChildren).
In
`@packages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsx`:
- Around line 232-252: The test "should render Layers icon when hasGroupChildren
is true" in CustomNodeContainer.test.tsx should scope the SVG assertion to the
collection badge: replace the broad container.querySelector('svg') check with a
scoped query such as container.querySelector('.step-icon__collection svg') so
the expectation ensures the Layers icon is rendered inside the
.step-icon__collection element rendered by the CustomNodeContainer component
when hasGroupChildren is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f875a9b-4c06-42b6-98e1-2c2a220b8b0f
📒 Files selected for processing (5)
packages/ui/src/components/Visualization/Custom/FloatingCircle/FloatingCircle.scsspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.tsx
matheusandre1
force-pushed
the
feature-2958
branch
from
e6b10d1 to
36b65f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.tsx`:
- Around line 57-68: The Layers icon is decorative but conveys semantics; update
the Icon wrapping the <Layers /> (inside the isCollapsed && childCount check) to
include an accessible label so screen readers know it represents nested
collections—e.g., add an aria-label or title like "contains nested collections"
(or a visually hidden span) on the Icon component used with hasGroupChildren
within CustomNodeContainer (the FloatingCircle / Icon / Layers block) so the
meaning is announced alongside the childCount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b51a772f-f7a0-41b2-940a-ddeddafee547
📒 Files selected for processing (5)
packages/ui/src/components/Visualization/Custom/FloatingCircle/FloatingCircle.scsspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx
- packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx
- packages/ui/src/components/Visualization/Custom/Node/CustomNodeContainer.test.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3136 +/- ##
==========================================
- Coverage 91.88% 91.87% -0.01%
==========================================
Files 606 606
Lines 23350 23353 +3
Branches 5534 5344 -190
==========================================
+ Hits 21454 21456 +2
- Misses 1786 1895 +109
+ Partials 110 2 -108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello @matheusandre1 ! Thanks for trying to address this issue. I have been testing it, but sadly it didn't work out. The icon is not showing when expected. If you run this branch and try to create a route with for example a choice and collapse it, you should see the label with the counter. Then, if you insert a second choice inside, you should see that number increased. If the code works, there should appear the layer icon next to the number, but it is not showing. The main issues are related to hasGroupChildren, because the isGroup parameter is not checking for nested child inside groups, and isCollapsed, because for example a choice is rendered as a regular node. |
|
This pull request is stale because it has been open for 14 days with no activity. It will be closed if there is no activity in the next 14 days. |
github-actions
Bot
added
stale
matheusandre1
force-pushed
the
feature-2958
branch
from
36b65f7 to
7bb1d54
Compare
|
2 participants
Closes: #2958
Summary by CodeRabbit
Style
Bug Fixes
New Features
Tests