Skip to content

feat(tree-select): reorder selected chips in place + reusable drag-drop primitive#389

Open
PieterjanDeClippel wants to merge 7 commits into
masterfrom
feature/tree-select-chip-reorder
Open

feat(tree-select): reorder selected chips in place + reusable drag-drop primitive#389
PieterjanDeClippel wants to merge 7 commits into
masterfrom
feature/tree-select-chip-reorder

Conversation

@PieterjanDeClippel

Copy link
Copy Markdown
Member

Why

The additional-samples/tree-select-drag-drop demo was supposed to let users reorder the selected items inside bs-tree-select itself, but it shipped a workaround: a separate cdkDropList bar rendered below the component, round-tripping order through [value].

Root cause: in multiple/checkbox mode the chips render as <span class="ts-chip"> inside the web component's shadow DOM (mp-tree-select.ts renderChips()). Angular CDK's cdkDrag/cdkDropList only bind to light-DOM Angular owns — they can't reach the shadow chips.

What

  • Chips now reorder in place inside the component (pointer drag, or keyboard: focus a chip → M → arrows → Enter). The separate bar is gone.
  • The drag mechanism is generalized into a reusable, framework-agnostic primitive @mintplayer/web-components/drag-drop (SortableController + CDK-parity moveItemInArray/transferArrayItem) — usable by any WC, not just tree-select.
  • Opt-in & tree-shakeable: the base mp-tree-select/BsTreeSelectComponent import no drag code — only a tiny register/get seam. SortableController is pulled in only when a consumer imports the opt-in artifact; otherwise reorderable is inert and the whole module is tree-shaken out.
    • Angular: BsTreeSelectReorderDirective from @mintplayer/ng-bootstrap/tree-select/reorder
    • WC / React / Vue: @mintplayer/web-components/tree-select-reorder (thin re-exports per wrapper)
  • New reorderable attribute/input + reorder event across the WC and all three wrappers (reorder also flows through the form value via value-change/CVA/v-model). No @angular/cdk dependency.

PRD: docs/prd/tree-select-chip-reorder.md.

Verification

  • drag-drop unit tests (geometry, moveItemInArray/transferArrayItem, keyboard reorder): 14 pass
  • Full web-components suite (regression): 772 pass / 52 files
  • Tree-shaking: built mp-tree-select chunk has 0 references to SortableController; the opt-in entries link it; both ./drag-drop and ./tree-select-reorder subpaths are in the package exports
  • nx build green for web-components, ng-bootstrap (incl. nested tree-select/reorder secondary entry), react, vue, and the demo app

Not covered

The live pointer-drag DOM interaction (ghost/indicator/hit-testing) is covered by code + the geometry/keyboard unit tests, but not yet by a browser (Playwright) e2e — that path is hard to exercise in jsdom. Happy to add one as a follow-up.

Out of scope (follow-ups)

  • Cross-list transferArrayItem drags; tree-node reorder/reparent in the dropdown panel
  • Migrating dock/tile-manager/query-builder/scheduler onto the shared primitive

🤖 Generated with Claude Code

PieterjanDeClippel and others added 7 commits June 30, 2026 13:58
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New @mintplayer/web-components/drag-drop entrypoint: SortableController
(Lit ReactiveController — pointer drag with mouse threshold / touch
long-press, floating ghost + drop indicator, M-key keyboard reorder),
plus CDK-parity moveItemInArray / transferArrayItem helpers. Data-free:
emits a single onDrop, the host mutates its own model. 14 unit tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mp-tree-select gains a `reorderable` attribute and a `reorder` event.
Reordering the selected chips (multiple/checkbox) is wired through a
tiny register/get seam (sortable-registry) so the base bundle imports
no drag code — `reorderable` is inert until a consumer opts in. New
@mintplayer/web-components/tree-select-reorder entry registers the
SortableController; verified the mp-tree-select chunk tree-shakes the
sortable code (0 refs) while the opt-in entry links it. Keyboard
reorder announced via the a11y live-announcer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rective

Base BsTreeSelectComponent gains a `reorderable` flag passthrough and a
`reordered` output (reorder also flows through the form value via CVA) —
no drag code in the base entry. New @mintplayer/ng-bootstrap/tree-select/reorder
secondary entry ships BsTreeSelectReorderDirective (selector
bs-tree-select[reorderable]); its constructor calls enableTreeSelectReorder()
to register the sortable factory. Importing the directive is the opt-in
that bundles the drag-drop code.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
React adds an onReorder event mapping; Vue adds a reorderable prop, a
reorder emit, and flows the reordered order back through v-model. Both
gain a thin `tree-select-reorder` opt-in entry that re-exports
enableTreeSelectReorder so consumers pull in the drag-drop module only
when they want it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrites additional-samples/tree-select-drag-drop to use the new
`reorderable` attribute + BsTreeSelectReorderDirective opt-in instead of
the separate CDK drop-list below the component. Also makes the wrapper's
`reorderable` input accept a bare boolean attribute (booleanAttribute
transform) so `<bs-tree-select reorderable>` typechecks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements in-component chip reordering for the BsTreeSelect component using a new framework-agnostic, pointer-event-driven SortableController primitive, replacing the previous external Angular CDK workaround. The feedback identifies several critical improvements: adding a disabled callback to dynamically handle toggling the reorderable state, optimizing pointer-move performance by avoiding layout thrashing, ensuring the drag indicator inherits the host's theme color, correctly implementing the Escape key to cancel keyboard reordering, and removing redundant model updates in the Angular and Vue wrappers.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +11 to +17
export interface TreeSelectSortableOptions {
items: () => readonly TreeNode[];
itemId: (node: TreeNode) => string;
onDrop: (event: { previousIndex: number; currentIndex: number }) => void;
label?: (node: TreeNode) => string;
announce?: (message: string) => void;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When reorderable is dynamically toggled from true to false, the SortableController remains attached and active because it does not receive a disabled state. To fix this, add a disabled callback option to TreeSelectSortableOptions so the controller can dynamically check if reordering is active.

Suggested change
export interface TreeSelectSortableOptions {
items: () => readonly TreeNode[];
itemId: (node: TreeNode) => string;
onDrop: (event: { previousIndex: number; currentIndex: number }) => void;
label?: (node: TreeNode) => string;
announce?: (message: string) => void;
}
export interface TreeSelectSortableOptions {
items: () => readonly TreeNode[];
itemId: (node: TreeNode) => string;
onDrop: (event: { previousIndex: number; currentIndex: number }) => void;
label?: (node: TreeNode) => string;
announce?: (message: string) => void;
disabled?: () => boolean;
}

Comment on lines +612 to +618
this._sortableHandle = factory(this, {
items: () => [...this._selected.values()],
itemId: (node) => node.id,
label: (node) => node.label,
announce: (message) => this.announcer.announce(message),
onDrop: ({ previousIndex, currentIndex }) => this.onReorderDrop(previousIndex, currentIndex),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Pass the disabled callback to the sortable factory so that the underlying SortableController can dynamically disable its pointer and keyboard listeners when reorderable is set to false.

      this._sortableHandle = factory(this, {
        items: () => [...this._selected.values()],
        itemId: (node) => node.id,
        label: (node) => node.label,
        disabled: () => !this._reorderable,
        announce: (message) => this.announcer.announce(message),
        onDrop: ({ previousIndex, currentIndex }) => this.onReorderDrop(previousIndex, currentIndex),
      });

Comment on lines +17 to +24
const controller = new SortableController(host, {
items: options.items,
itemId: options.itemId,
onDrop: options.onDrop,
label: options.label,
announce: options.announce,
axis: 'both',
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Forward the disabled option from the registry options to the SortableController instance to ensure dynamic state changes are respected.

Suggested change
const controller = new SortableController(host, {
items: options.items,
itemId: options.itemId,
onDrop: options.onDrop,
label: options.label,
announce: options.announce,
axis: 'both',
});
const controller = new SortableController(host, {
items: options.items,
itemId: options.itemId,
onDrop: options.onDrop,
label: options.label,
announce: options.announce,
disabled: options.disabled,
axis: 'both',
});

Comment on lines +238 to +248
private updateDrag(clientX: number, clientY: number): void {
if (this.ghost) {
this.ghost.style.left = `${clientX - this.grabOffsetX}px`;
this.ghost.style.top = `${clientY - this.grabOffsetY}px`;
}

const others = this.itemElements().filter((el) => el !== this.sourceEl);
const boxes = others.map((el) => el.getBoundingClientRect());
this.dropIndex = resolveDropIndex(boxes, clientX, clientY, this.axis);
this.positionIndicator(others, boxes);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Writing to this.ghost.style before calling getBoundingClientRect() on the list items causes a forced synchronous layout (layout thrashing) on every single pointermove event. Reordering these operations to read all layout properties first before writing style updates completely eliminates this performance bottleneck.

Suggested change
private updateDrag(clientX: number, clientY: number): void {
if (this.ghost) {
this.ghost.style.left = `${clientX - this.grabOffsetX}px`;
this.ghost.style.top = `${clientY - this.grabOffsetY}px`;
}
const others = this.itemElements().filter((el) => el !== this.sourceEl);
const boxes = others.map((el) => el.getBoundingClientRect());
this.dropIndex = resolveDropIndex(boxes, clientX, clientY, this.axis);
this.positionIndicator(others, boxes);
}
private updateDrag(clientX: number, clientY: number): void {
const others = this.itemElements().filter((el) => el !== this.sourceEl);
const boxes = others.map((el) => el.getBoundingClientRect());
this.dropIndex = resolveDropIndex(boxes, clientX, clientY, this.axis);
if (this.ghost) {
this.ghost.style.left = `${clientX - this.grabOffsetX}px`;
this.ghost.style.top = `${clientY - this.grabOffsetY}px`;
}
this.positionIndicator(others, boxes);
}

Comment on lines +320 to +331
private buildIndicator(): HTMLElement {
const el = document.createElement('div');
el.className = 'mp-sortable-indicator';
Object.assign(el.style, {
position: 'fixed',
zIndex: '2147483646',
background: 'currentColor',
pointerEvents: 'none',
borderRadius: '1px',
} satisfies Partial<CSSStyleDeclaration>);
return el;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since the indicator is appended directly to document.body, using background: 'currentColor' means it will fall back to the body's text color instead of inheriting the host component's theme color or custom text color. Reading the computed color of the host element and applying it to the indicator ensures visual consistency across different themes.

Suggested change
private buildIndicator(): HTMLElement {
const el = document.createElement('div');
el.className = 'mp-sortable-indicator';
Object.assign(el.style, {
position: 'fixed',
zIndex: '2147483646',
background: 'currentColor',
pointerEvents: 'none',
borderRadius: '1px',
} satisfies Partial<CSSStyleDeclaration>);
return el;
}
private buildIndicator(): HTMLElement {
const el = document.createElement('div');
el.className = 'mp-sortable-indicator';
const hostColor = getComputedStyle(this.host).color;
Object.assign(el.style, {
position: 'fixed',
zIndex: '2147483646',
background: hostColor || 'currentColor',
pointerEvents: 'none',
borderRadius: '1px',
} satisfies Partial<CSSStyleDeclaration>);
return el;
}

Comment on lines +388 to +395
// grabbed
if (key === 'Escape' || key === 'Enter' || key === 'm' || key === 'M') {
event.preventDefault();
const droppedId = this.grabbedId;
this.grabbedId = null;
this.announceFor(droppedId, (label, pos, total) => `${label} dropped. Position ${pos} of ${total}.`);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The PRD states that the Escape key should cancel the keyboard reorder. However, the current implementation treats Escape exactly like Enter or M, committing the drop at the current position and announcing it as 'dropped'. Since the host array is mutated on every arrow key press, cancelling would require keeping track of the original index when grabbed and reverting the move if Escape is pressed.

Comment on lines +169 to +175
onReorder(event: Event): void {
const detail = (event as CustomEvent<TreeSelectReorderEventDetail>).detail;
// Reorder changes the selection order; flow it back through the form value.
this.value.set(detail.value);
this.onChange(detail.value);
this.reordered.emit(detail);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since value-change is already fired on reorder drops and updates the form value/CVA via onValueChange, updating the value and calling onChange again in onReorder is redundant and triggers duplicate change detection cycles and form validations. onReorder should only emit the semantic reordered output.

  onReorder(event: Event): void {
    const detail = (event as CustomEvent<TreeSelectReorderEventDetail>).detail;
    this.reordered.emit(detail);
  }

Comment on lines +175 to +181
function onReorder(ev: Event) {
const detail = (ev as CustomEvent<TreeSelectReorderEventDetail>).detail;
if (!detail) return;
// Reorder changes the selection order; flow it back through v-model.
model.value = detail.value;
emit('reorder', detail);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Updating model.value in onReorder is redundant because onValueChange already handles updating the model value for all selection and reordering changes. onReorder should only emit the reorder event.

function onReorder(ev: Event) {
  const detail = (ev as CustomEvent<TreeSelectReorderEventDetail>).detail;
  if (!detail) return;
  emit('reorder', detail);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant