Skip to content

feat!: Tree component rework#783

Open
shahzeelahmed wants to merge 45 commits into
mainfrom
tree-rework
Open

feat!: Tree component rework#783
shahzeelahmed wants to merge 45 commits into
mainfrom
tree-rework

Conversation

@shahzeelahmed

@shahzeelahmed shahzeelahmed commented Jun 13, 2026

Copy link
Copy Markdown
Member

Coverage: 57.42% (+0.32% vs main)

Docs preview: https://ui.frappe.io/pr-preview/pr-783/docs/components/tree

The Tree was rebuilt from a single recursive node renderer into a stateful forest. It takes a nodes array, drives expansion through v-model:expanded, and scopes its per-row slots as #item-*. The options blob and the per-node internal collapse state are gone and sizing moves to CSS variables. Keyboard navigation, role="tree" ARIA, and opt-in drag-and-drop are new.

Before After
:node="root" (single root object) :nodes="[root]" (array of roots)
internal per-node collapse v-model:expanded (array of keys)
:options="{ rowHeight, indentWidth }" --tree-row-height / --tree-indent CSS vars
:options="{ showIndentationGuides }" guides="connectors" | "lines" | "none"
:options="{ defaultCollapsed: true }" defaultExpanded (inverted)
#node="{ node, isCollapsed, toggleCollapsed }" #item="{ node, expanded, toggle, … }"
#label #item-label
#icon built-in chevron; override via #item
<!-- Before -->
<Tree :node="root" node-key="name" :options="{ defaultCollapsed: false }">
  <template #label="{ node }">{{ node.title }}</template>
</Tree>

<!-- After -->
<Tree :nodes="[root]" node-key="name" default-expanded>
  <template #item-label="{ node }">{{ node.title }}</template>
</Tree>

Drag-and-drop is opt-in: set draggable, gate drops with :move="({ node, target, position }) => …", and persist from @drag-end="(info) => …"info is{ node, from, to, position, oldIndex, newIndex }, or null when the drag is cancelled.

@shahzeelahmed shahzeelahmed changed the title feat: stories for tree rework feat: Tree component rework Jun 13, 2026
@barista-for-frappe

Copy link
Copy Markdown

Minor nits — solid rework, and it moves the API in the right direction. Nothing blocking.

Good direction worth calling out:

  • The old options config blob is gone, replaced by flat props (guides, rowHeight, indent, …) — exactly what P3 wants.
  • The old #icon slot is dropped in favor of canonical #prefix/#suffix (TreeItem.vue:68,:78), plus a canonical #empty (Tree.vue:36). That's P11/P6.
  • Tests and stories are thorough (tree.cy.ts covers expansion, selection, keyboard, ARIA, slots, guides, DnD, canDrop).

Nits:

  • Tree.api.md is stale — it still documents the old node/options props. It's auto-generated, so just re-run scripts/propsgen.ts; otherwise the published props table will be wrong.
  • useTreeDragDrop.ts:144-146 — the emitted MoveEvent.index is computed against siblings that still contain the dragged node. When reordering before/after within the same parent and moving downward, a caller that removes the source first lands one slot off. Worth a doc note or adjusting the index, since the docstring promises the 'final index'.
  • types.ts:100,:106rowHeight/indent are free CSS strings that just set --tree-row-height/--tree-indent. Consumers can already override those vars via CSS, so the props are partly redundant — fine to keep for ergonomics, just flagging.

Note this is a full breaking change to Tree's public API. Pre-v1 that's allowed (P13 isn't on yet), but it deserves a changelog line.

@shahzeelahmed

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shahzeelahmed.

Looks good — prior nits are addressed and the API moves the right way.

Resolved since last pass:

  • Tree.api.md is regenerated — now documents nodes/labelKey/childrenKey/etc. and the emits table.
  • The move index off-by-one is fixed: useTreeDragDrop.ts:1876-1883 decrements insertIndex on same-parent downward reorders, and there's a test pinning it (tree.cy.ts 'reports the final post-removal index when reordering down').
  • rowHeight/indent kept for ergonomics — fine.

Slots are all canonical (#node/#label/#prefix/#suffix/#empty), #icon dropped, expand/select on v-model. Good.

Two small nits, nothing blocking:

  • Tree.vue template now has three root nodes (the <ul>, the drag <Teleport>, and the sr-only live region). With multiple roots Vue can't auto-inherit attrs, so <Tree class=\"...\"> silently won't apply (the stories already wrap in a w-80/w-96 div, which hints at this). Worth designating fallthrough on the <ul> via inheritAttrs: false + v-bind=\"\$attrs\", or noting it.
  • P7: #prefix gets { node, expanded, hasChildren } but not selected, while #label/#suffix/#node all expose selection state. A prefix (the ListView avatar) can't react to its row being selected without re-deriving it. Add selected to #prefix for consistency.

Still a full breaking change to Tree's public API — fine pre-v1, but it deserves a changelog line.

@shariquerik

Copy link
Copy Markdown
Member

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik.

Minor nits — the API is in good shape (canonical #prefix/#suffix/#label/#empty slots, v-model:expanded/v-model:selected, flat props, data-slot+CSS for guides per P10). The two nits from the last pass still look open in the current diff, plus CI is red:

  • CI: Component Tests (Cypress) - 1 is failing. Worth checking it's not tree.cy.ts before merge.
  • Tree.vue:1 — template still has three roots (the <ul>, the drag <Teleport>, the sr-only live region). With multiple roots Vue can't auto-inherit attrs, so <Tree class=\"...\"> silently won't apply. Add inheritAttrs: false + v-bind=\"$attrs\" on the <ul>.
  • TreeItem.vue:68#prefix still gets { node, expanded, hasChildren } but not selected, while #label/#suffix/#node all expose it (P7). A prefix (e.g. the ListView avatar) can't react to its row being selected without re-deriving it.

The move-index off-by-one fix (useTreeDragDrop.ts:144-150) and the regenerated Tree.api.md both look good.

Still a full breaking change to Tree's public API — fine pre-v1 (P13 not on yet), but it deserves a changelog line.

@shahzeelahmed

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shahzeelahmed.

Minor nits — API still in good shape (canonical #prefix/#suffix/#label/#empty, v-model:expanded/v-model:selected, flat props, sizing moved to CSS vars per P10). Both open nits from the last two passes are still unaddressed:

  • Tree.vue:1 — template still has three roots (the <ul>, the drag <Teleport>, the sr-only live region). No defineOptions({ inheritAttrs: false }) + v-bind=\"$attrs\" on the <ul>, so <Tree class=\"...\"> silently won't apply. (The stories sidestep it by wrapping in a sized div.)

  • TreeItem.vue:68#prefix still gets { node, expanded, hasChildren } but not selected, while #label and #suffix both expose it (P7). A prefix (e.g. a ListView avatar) can't react to its row being selected without re-deriving it. Add selected to the #prefix bind and to its type in types.ts/Tree.vue.

  • CI: the Component Tests (Cypress) shards are re-running right now (shard 2 passed, 1/3 pending). The shard that was red last pass isn't confirmed green yet — worth a glance before merge.

Still a full breaking change to Tree's public API — fine pre-v1 (P13 not on yet), but it deserves a changelog line.

@shahzeelahmed shahzeelahmed marked this pull request as draft June 15, 2026 11:38
@shahzeelahmed shahzeelahmed marked this pull request as ready for review June 23, 2026 20:43
@barista-for-frappe

Copy link
Copy Markdown

Concerns — the per-row slots use top-level names for a per-item concept.

  • Tree.vue / types.ts#node, #prefix, #label, #suffix are rendered once per node (a repeated unit), but use the bare top-level slot names. P6 says repeated-unit slots must be scoped by the unit. The selection family already settled this: #item-prefix / #item-label / #item-suffix (Select.vue:307,429,452, Select/types.ts:120-126). Rename Tree to use #item, #item-prefix, #item-label, #item-suffix. Bare #prefix/#suffix are meant for whole-component leading/trailing, so this also keeps them free for later. (Earlier passes called these "canonical" — they are not, for per-item slots; worth fixing now while there are no consumers.)

Other items:

  • Tree.vue:1 — three root nodes (<ul>, drag <Teleport>, sr-only live region). With multiple roots Vue cannot auto-inherit attrs, so <Tree class="..."> silently will not apply — and root class fallthrough is the sanctioned styling path (P10). Add defineOptions({ inheritAttrs: false }) + v-bind="$attrs" on the <ul>. (Open from prior passes.)
  • TreeItem.vue:740-747#suffix gets only { node } and #prefix has no focused/disabled (P7). A row action or avatar cannot react to its row state without re-deriving it. Pass the row state through.
  • Tree.vue:618defaultExpanded seeds once in onMounted. If nodes load async (empty at mount), it never seeds. Consider seeding on the first non-empty roots.
  • nit: useTreeDragDrop captures move: props.move at setup, so a later predicate swap will not take effect.

Good direction: dropping the options config blob for flat props + CSS vars (P3, P10), v-model:expanded (P2), solid ARIA tree + keyboard nav (P12), composable extraction, and full tests + stories/docs. The breaking rewrite is fine pre-v1 but deserves a changelog line. CI is green now.

@shahzeelahmed

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shahzeelahmed.

Looks good — every open concern from the last pass is addressed, and the API moved the right way.

Resolved since last review:

  • Per-item slots renamed to #item / #item-prefix / #item-label / #item-suffix (P6) — matches the Select family (Select.vue:307).
  • Multi-root attrs fixed: defineOptions({ inheritAttrs: false }) + v-bind="$attrs" on the <ul> (Tree.vue), so <Tree class="..."> now lands on the tree (P10).
  • P7 slot state: all per-item slots now get the same itemSlotProps (node, level, expanded, hasChildren, focused, disabled) — #item-prefix/#item-suffix no longer have to re-derive row state. (Selection was dropped entirely, so the old selected ask is moot.)
  • defaultExpanded async seeding fixed — now a watch(roots, …) that seeds on the first non-empty roots, with a test pinning it ('seeds defaultExpanded when nodes arrive asynchronously').
  • move predicate is now read live (move: (ctx) => props.move?.(ctx) ?? true), not snapshotted at setup.

Two non-blocking things:

  • CI: Component Tests (Cypress) - 1 is failing. Worth confirming it isn't tree.cy.ts before merge.
  • Still a full breaking rewrite of Tree's public API — fine pre-v1 (P13 not on yet), but it deserves a changelog line.

@shahzeelahmed shahzeelahmed changed the title feat: Tree component rework feat!: Tree component rework Jun 24, 2026
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.

2 participants