Feat/groups#63
Merged
Merged
Conversation
Introduces the structural primitive for grouping: GroupObject is a non-leaf node carrying children: LabelObject[]. LabelObject becomes LeafObject | GroupObject so consumers can narrow with isGroup. No render, store, or UI wiring yet. Registry stays leaf-only by design: groups have no toZPL, no defaultSize, no PropertiesPanel. Helpers (walkObjects, getAllLeaves, findObjectById, findAncestors) are the surface every future consumer talks to instead of recursing ad-hoc.
groupSelection wraps every selected top-level, unlocked object in a new GroupObject, inserted at the topmost selected position so the group lands where the user's eye is. ungroup splices a selected group's children back at its index. V1 scope: top-level only. Grouping items already inside another group, or ungrouping nested groups, is out of scope until selection behaviour for nested children lands. No UI binding yet. Consumers (render, ZPL export, layers panel) are unchanged because no UI path can create a group; subsequent commits wire those up alongside the keyboard shortcut.
ZPL export recurses through groups so wrapping leaves changes nothing in the output. includeInExport=false on a group skips the whole subtree; otherwise per-leaf includeInExport still applies. Visible flag cascades the same way on the canvas render path. LayersPanel intentionally untouched: it operates on top-level objects and reorderObject works on top-level indices. A group renders as a plain row with type 'group' for now — hierarchy-aware layers are a later commit.
Ctrl+G calls groupSelection, Ctrl+Shift+G calls ungroup. Listed before the bare-G grid toggle and the bare branch now requires no modifier so the two no longer collide. First user-visible step of the groups feature: keyboard shortcut exists and creates persisted groups, even though selection and drag behaviour still treat children individually.
Selecting a group crashed the canvas because getStepRotation reached
through obj.props on a node that has none. The rotate-button overlay
calls it on every selected object; before this fix, the crash also
left selectedIds pointing at the now-existing group, so subsequent
clicks reopened the same code path and selection felt broken.
Loosens the helper's input type to { props?: object } so the same
fallback (return null) covers leaves with no rotation prop and
groups with no props field at all.
…leaves Click on a child of a group now selects the outermost containing group (Figma-style auto-select-parent). The store keeps the intent-level selection (group id), while the Konva transformer, multi-drag propagation, lasso, and per-frame snap all walk an 'attachable' projection of the selection — the flat list of leaf ids that actually own Konva nodes. The store gains mapObjectById for tree-walking writes so updateObject and updateObjects reach leaves nested inside groups; drag and arrow keys both rely on this. Snap during a child's drag iterates getAllLeaves so it sees siblings outside the group as snap targets, matching the ungrouped behaviour. Lasso intersects leaf nodes and promotes hits to their outermost group, so a marquee over grouped children selects the group as one unit instead of a fragmented multi-selection. A group's lock cascades to its children for the lasso opt-out only; further lock cascade is out of scope for v1.
…ys / align A locked group now actually behaves locked: lock cascades into descendants for click handling (the locked-click passthrough fires when the resolved selection target is locked, not just the clicked leaf) and for drag (visibleLeaves stamps an effective locked flag onto leaves inside a locked group so the per-leaf draggable check sees one consistent value). Arrow keys and align-to-label now expand the selection through groups the same way drag does, so moving a selected group with the keyboard or aligning it to the label actually shifts its children instead of writing to the group's conventionally-zero x/y.
Both PropertiesPanel header and LayersPanel row now show a folder glyph plus the localised 'Group' label instead of rendering blank (no registry icon) and falling back to the raw type-string 'group'. Adds the 'group' key to all 32 locales via the standard add_locale_key.local.py path so the entry lands consistently.
The layers panel renders the object tree instead of just the top-level array: group rows get a chevron toggle, expanded groups show their children indented, and each group row carries an ungroup button that does not touch the active selection. Click on a child row selects that child directly (Figma drill-in), distinct from canvas clicks where auto-select-parent still surfaces the containing group as the selection target. V1 scope: drag-reorder still operates on top-level only. Nested rows render but are intentionally outside the SortableContext; dragging items in or out of groups is the next commit.
Every visible row in the layers panel is now sortable and carries
its container id. Dragging a row onto a sibling reorders it; dragging
onto a collapsed group drops the item into that group; dragging into
the visible children of an expanded group lands the item beside its
new siblings; dragging a child back to a top-level position extracts
it from its group.
The drop target gets a soft accent outline so the user sees which
group will receive the dragged item.
Store gains reparentObject(id, { parentId, index }) backed by
detachObjectById and an isSelfOrDescendant cycle check that prevents
moving a group into one of its own descendants. Both helpers live
alongside the existing tree-walk utilities in types/Group.
While dragging a layer row, the panel now distinguishes the two drop modes: - An accent insertion line above the over-row signals a sibling drop. The line is indented to the over-row's nesting level so the user can see at which depth the item will land. - A soft outline on the over-row body signals a drop INTO a collapsed group; this stays separate from the insertion line so the two intentions don't visually compete.
…on line Two related layers-panel bugs: - A group that lost its last child could not be a drop target again. The drop-into rule fired only on collapsed groups; expand-state is moot when the group has no children to drop between, so the rule now also matches empty groups regardless of their expanded flag. The 'into' outline highlights the same case. - The insertion line and the actual landing slot disagreed when dragging downward. The line was rendered in the gap above the over-row, but the drop used over's data index, which (after the display reversal) lands the active at over's display slot with over shifted up. Switched the sibling formula to insert in the gap ABOVE over in display: insertionIndex = overDataIndex + 1 for cross-container or active-below-over drops, overDataIndex when same-container with active above over (detaching shifts over's effective index by one).
The bottom-most row in the panel had no row below it to use as a drop target, so a child of a group sitting at the very end of display had no way to leave the group by dragging downward. Added a thin droppable zone after the last row that maps to top-level insertion at data index 0 — the same position the user would reach by dropping on a hypothetical row 'after everything'. The zone highlights softly while a drag hovers it.
The bottom drop zone was the wrong shape for this problem — it's a one-off UI element that only addresses the bottom-of-panel case and leaves the broader 'where does this row land' question to the user's guess. Replaced with the indent-drag idiom every tree-aware design tool uses (Figma, Sketch, VSCode): while dragging, the cursor's X position selects how deep in the container chain the drop lands, and the rendered insertion line slides left or right in real time to preview the target depth. Mechanics: capture clientX at drag start, follow it via dnd-kit's onDragMove deltas, quantise (cursorX - INDENT_DEAD_ZONE) / INDENT_STEP into a depth, clamp to the over row's own depth, then climb the container chain by (overDepth - effectiveDepth) levels. The climbed group node becomes the effective over for insertion-position math; the line renders at the climbed depth so the user sees the future landing slot. Same store reparentObject path handles the write.
The previous version derived cursor X from dnd-kit's activatorEvent plus the drag-move delta, which silently fails when activatorEvent has no clientX (synthesised events, some sensor configurations) — dragCursorX never updated, the indent path stayed dormant and the new behaviour felt indistinguishable from the old one. Replaced with a direct document-level pointermove listener that runs only while a drag is active. Cursor X relative to the panel feeds the existing depth-from-X logic unchanged.
The active-equals-over early return blocked the most common indent-drag motion: grabbing a row and pulling it left without moving up or down so the cursor stays on the row's own gap. The preview line correctly slid to depth 0, but the release was discarded as a no-op and the row stayed where it was. Reworked the guard around resolveDropTarget's effective overObj: when indent climbing produced an ancestor as the effective over, the drop now proceeds even though active.id === over.id at the row level. The pure no-op (same row, no climb) still short-circuits exactly as before.
…strings LayerRow was getting eight translation strings (tLock, tUnlock, tShow, tHide, tGroup, tExpand, tCollapse, tUngroupLabel) drilled through its props purely for display purposes. The rest of the codebase calls useT() inside the consuming component for exactly this case. Switched to that pattern: LayerRow's prop surface is now back to data + handlers.
The layers panel was carrying its drag-and-drop machinery inline: cursor tracking, the document-level pointermove listener, climbContainer / resolveDropTarget helpers (rebuilt every render as closures), four DndContext handlers, and the live preview derivation. Pulled all of it into a dedicated hook with a small return surface (sensors, panelRef, four handlers, a preview object). climbContainer and resolveDropTarget moved to module-level pure functions taking objects + cursor as explicit parameters, so they no longer allocate per render. The preview is now memoised off the same resolution path that the on-release commit uses, which keeps the rendered insertion line guaranteed to match where the drop will land. LayersPanel is back to render + bulk-toggle + expand-state responsibilities.
The 'group with no expandable children to land between' rule was duplicated in both the commit path and the live preview derivation. Pulled it into a named helper so the predicate has a single home and the two callers can't drift.
Discoverability fix for the groups feature: there was no UI affordance to create a group — only Ctrl+G. Added a small FolderPlus button to a new header row in the layers panel. Smart click: when the current selection has any unlocked top-level item, it groups them (same as Ctrl+G); otherwise it creates an empty group at the top and selects it so the user can drag items into it via the existing indent drag. Store gains addGroup() for the empty-group case, alongside the existing groupSelection / ungroup / ungroupIds. The header stays visible on an empty design so the affordance is reachable up front.
LabelObjectBase gains an optional name field; for now only groups read it so the panel can show 'Header' instead of the generic 'Group' label. Other consumers see the field via the base schema without behaviour change, leaving leaf renaming as a future UI-only addition. Layers panel: double-clicking a group row replaces the label with an inline input. Enter commits, Escape cancels, blur commits. The input intercepts pointerdown so the sortable's drag activation doesn't kick in mid-rename. Empty input clears the name back to the default fallback. designFile loader relaxes the schema: props is now optional and children is accepted so designs containing groups survive a save/load round trip. The runtime registry was already permissive to unknown prop shapes, so no per-type schema work was needed.
…r path LayersPanel.tsx was carrying both the panel container (state, dnd wiring, header, sortable context) and the LayerRow component (sortable attachment, inline rename state, button bar) in one file. Split into LayerRow.tsx alongside the existing useLayerDnd.ts so each module has one concern. Panel drops to 128 lines, row stands alone at 213. Render structure: header and DndContext are now always present, with the empty-state message and the SortableContext + rows toggling inside the same outer wrapper. Removes the duplicated wrapping div the empty and full branches each had, and lets the new-group button sit at the same DOM level regardless of state.
Two UX nudges for the layers tree: A. Each nested row now renders one fixed-width spacer per ancestor level, each carrying a left border. Consecutive rows at the same depth line up so the borders read as continuous vertical guides from a parent group's row down through its children — same idiom as Figma / VSCode / Notion tree views. Replaces the old paddingLeft override; total left offset stays at depth*16+8 dots. B. When a group is selected, every descendant row gets a soft accent tint (bg-accent/5). Signals 'these move with me if you drag or arrow' before the user starts the gesture, which was the missing affordance once selection became group-level.
Two layer-panel UX nudges: B. Group rows render their label at font-medium so the eye latches onto them as containers, not as more peer rows next to their children. Subtle weight bump (500 vs 400) reads as a header without competing with the selection accent. G. Collapsed groups append the child count to their label as 'Name · 3', so the user can judge what's inside without expanding. Suppressed while editing (the input would otherwise prefill with the count too) and while the group is empty (zero would just be noise).
When the next row leaves a deeper container, the current row picks up a small bottom margin (mb-1) so the visual boundary between a group's children and the next sibling at the parent level is obvious. Without it the list flowed straight through and the user had to count indent to tell which rows still belonged to the group above and which were the next top-level item.
LayerRow's indent column had a depth-0 branch (single 8px span) and a depth>0 branch (flex wrapper + spacers). Both produced the same leading 8px offset. Unified to always render the wrapper; depth 0 just emits the gutter span with no extra spacers. One code path. labelStore.reparentObject was inlining 'splice an item into an array at a clamped index' twice — once for top-level inserts, once for into-group inserts. Pulled into a small insertAt<T>() helper next to the other store-private utilities. Reads cleaner and removes the visual duplication where the two branches looked nearly identical.
INDENT_STEP was exported from useLayerDnd.ts (the dnd hook) but consumed by both the hook (for cursor-X-to-depth math) and LayerRow (for the indent spacer column and the insertion line indent). The constant is a layout concern, not drag-protocol, so its home was wrong. Pulled into src/components/Properties/layerLayout.ts; both consumers now import from there and the hook no longer leaks a visual constant through its public surface.
Double-clicking a group row in the layers panel renames it, but that gesture is hidden. Surface the same field as a regular input at the top of the PropertiesPanel body when a group is selected. Empty value falls back to the localised 'Group' default — same semantics as the layers panel inline-rename so the two stay aligned.
When more than one item is selected the PropertiesPanel showed only the count, the arrow-keys hint and the align buttons — no discoverable way to act on the selection. Added a 'Group selection' button alongside Align, gated on whether any of the selected items is top-level and unlocked (the same gate groupSelection itself applies internally) so the control doesn't appear when it would be a no-op.
…TypePanel Two border-t separators bracketed the per-type TypePanel slot. With no registry entry for groups TypePanel rendered nothing, so the two separators landed flush against each other — two horizontal lines back to back with empty space neither above nor between. Tucked the trailing separator inside the TypePanel conditional so the line only appears when there's a type-specific section to close off.
…roupSelection Three follow-ups from the latest validation pass: - PropertiesPanel resolves selectedIds[0] via findObjectById instead of a top-level Array.find. When the layers panel drills into a child the selection holds a leaf id that isn't in the top-level list; the old lookup missed it and the panel silently fell back to LabelConfigPanel. Tree walk now matches. - canGroupSelection(objects, selectedIds) moves into types/Group as the single home for the 'is there anything groupRow would act on' predicate. Both the layers-panel header button and the multi-select properties button consume the same function instead of inlining the same .some/.some pair. - The group properties view hides the inputs that don't apply: x/y (a group's own coordinates are conventionally zero and don't drive children) and the ^FX comment (groups emit no ZPL). Align stays since it expands through the group at the canvas layer. Type-string equality checks switched to isGroup for consistency with the rest of the codebase.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive grouping system for label objects, allowing users to organize elements into hierarchical structures. Key changes include the addition of a new GroupObject type, a revamped layers panel with drag-and-drop support for reparenting, and updated canvas logic to handle group-based selection and transformations. The ZPL generator and property panels have also been updated to support recursive tree structures. Feedback focuses on optimizing performance, specifically by avoiding layout thrashing in the drag-and-drop pointer handler and reducing the complexity of tree-walking operations in the store and lasso selection logic through single-pass traversals.
…level CI build (tsc -b, not the no-op tsc --noEmit on the root config) caught the cluster of consumers that still typed their input as LabelObject even though they only handle leaves: - KonvaObjectProps.obj, the ghost preview, and useKonvaTransformer's internal types switch to LeafObject so the per-type renderers, Konva transformer attachment and snap snapshot can reach .props without narrowing. - getAllLeaves now returns LeafObject[] (semantically already true) so callers don't widen back to LabelObject. - buildBwipOptions / getDisplaySize / getEanUpcLayout accept LeafObject — barcodes only. - applyObjectChanges short-circuits on groups and skips the registry normalize / props merge path (groups have no props and no registry entry). - buildOffsetCopies, copySelectedObjects, duplicatePage clone groups deeply (children get fresh ids recursively) so duplicates don't collide with the source. - PropertiesPanel guards TypePanel render with the new groupRow narrow so the union doesn't widen back. - getStepRotation signature accepts the wider 'type+props?' shape and short-circuits on type==='group'. - Test helpers (props extractor, fixture leaf builder) accept the unioned shape too. Root cause: 'tsc --noEmit' on the root tsconfig is a no-op (files=[], references only). Local pre-commit was passing while CI's 'tsc -b' failed. Switching the local check to 'tsc -b' surfaces the same errors going forward.
Three findings, all valid: 1. useLayerDnd's document-level pointermove handler read getBoundingClientRect on every frame, forcing a synchronous layout pass. Cached the panel rect once at drag start and reused it during the move; reset on cancel/end. Trade-off: a mid-drag scroll would invalidate the cache, but layer-row drags don't scroll the panel. 2. updateObjects reduced over mapObjectById, giving O(updates × tree) complexity on a per-frame hot path (multi-object drag emits one update per moved leaf). Replaced with a single recursive tree walk that applies every queued change in one pass from a Map keyed by id — O(tree). 3. useCanvasLasso's leaf-id gather called selectionTargetId and findObjectById per leaf, both O(tree). Folded into one walk that builds the leaf list AND a Map<leafId, topAncestorId> in the same pass; the hit→selection promote step is now a Map lookup instead of another O(tree) walk per hit. Lock cascade is tracked inline through the recursion.
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces grouping functionality to the label design tool, allowing objects to be organized into recursive tree structures. Key changes include the addition of a GroupObject type, new store actions for grouping, ungrouping, and reparenting, and a revamped Layers panel that supports nested layers and drag-and-drop reordering. The ZPL generation and canvas rendering logic were also updated to handle the hierarchical object tree. Review feedback identified several critical issues: a UX conflict in the drag-and-drop logic where "drop into" prevents sibling reordering above collapsed groups, a bug in the same-container reordering index calculation, and a performance concern regarding unstable object references in the visibleLeaves memoization. Additionally, a more robust recursive Zod schema was suggested for better type safety during file parsing.
… formula, recursive schema Three findings from the follow-up review: - shouldDropInto now reads cursor depth so a collapsed non-empty group only acts as a drop-into target when the user pulls the cursor deeper than the group's row. Otherwise the drop falls through to the sibling path. Empty groups stay 'always into' — there's no other way to populate them. Expanded groups never drop-into here; the user drops on a child to land beside it. Extracted cursorDepthFor() so the preview and the commit path share the same X-to-depth quantisation. - Sibling-drop index simplifies to overDataIndex always. The previous +1-when-active-above-over branch produced two bugs: the bottom-most slot (data index 0) was unreachable via drag and a one-step neighbour cross became a no-op. With the always-overDataIndex rule, dropping on a row puts active at that row's position and the displaced row shifts up — full sortability restored. - designFile schema switches to z.lazy() so children are validated recursively as labelObjectSchema instead of opaque z.unknown(). Schema is genuinely typed now; bad nested shapes get caught at parse time.
The commit path and the preview useMemo each ran the same three-line sequence — cursorDepthFor → shouldDropInto branch → resolveDropTarget — with the isGroup check inlined twice. Folded into a single resolveDropMode() returning a discriminated union: 'into' carries the target group, 'sibling' carries the resolved slot. Both consumers just match the union; a future drop-mode variant lands in one place instead of two. Three small follow-ons fell out of the move: - resolveDropTarget now calls cursorDepthFor for its quantisation instead of inlining the Math.floor expression a second time. - The cross-container drop path's IIFE for resolving the parent's children list lifts into containerChildrenOf(objects, parentId). - onDragEnd reads cleaner: one switch on mode.kind, then a small guard cluster around the sibling slot.
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces object grouping functionality to the label editor, allowing users to organize, select, and transform multiple objects as a single unit. Key implementation details include a recursive GroupObject structure, a redesigned hierarchical layers panel with drag-and-drop reparenting, and updated canvas logic for group-aware interactions. The ZPL generation and design file schema have also been extended to support nested trees. Review feedback highlights opportunities to optimize tree-walking functions in the store and group utilities by preserving object identity for unchanged nodes, which is critical for maintaining React component memoization and performance during frequent updates like dragging.
Both tree walks always returned new arrays / new group nodes even when no descendant actually changed. That broke React identity for the unaffected portions of the tree, defeating any downstream memoisation. Added the 'changed' flag pattern: the function returns the original array when nothing in it (or below it) was touched, otherwise the rebuilt one. Group nodes likewise keep their reference when their children walk returns by reference. Picked up four Group.test cases to lock the identity behaviour: - mapper applied at the matching node - top-level array preserved when id is missing - untouched group subtrees preserved by reference - mapper that returns the same node leaves the array untouched
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive grouping feature, allowing label objects to be organized into hierarchical structures. Key changes include updates to the canvas rendering logic, a new interactive layers panel with drag-and-drop reparenting, and support for recursive group schemas in design files. Feedback from the review identifies a potential bug in the inline renaming logic where blur events could trigger unintended commits during cancellation, and suggests optimizing performance in the layers panel by reducing re-renders during drag operations.
LayerRow's escape handler set editing=false, which unmounted the input and triggered a synthetic blur in browsers that fire blur on unmount. That landed in commitEdit and persisted any unsaved draft text the user meant to discard. Added a cancellingRef the blur path now checks before running the save.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.