Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate the list keys with the hierarchy reference #185

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lsfgrd
Copy link

@lsfgrd lsfgrd commented Nov 2, 2023

This is a follow up to solve the issue mentioned on #175.

With this change, instead of just using the node id for the react key, two new methods were created in the Node API:

  • path(): This method constructs an array that captures all the IDs in the node's hierarchy, from the root to the node itself.
  • key(): The key() method takes the array of IDs generated by path() and combines them to create the actual key to be used by react-window's FixedSizeList.

This change ensures that keys will remain unique, even in scenarios where data may contain nodes with the same ID under different parent nodes. By considering all ancestors in the key generation process, we prevent potential conflicts.

Keys will now look something like this if they are nested: __REACT_ARBORIST_INTERNAL_ROOT__,12,15,15-1

@jameskerr
Copy link
Member

This is awesome. Thank you. Two requests:

  1. Instead of deriving the path in the node, I'd like to pass it into the NodeApi constructor as a param. This way we do them all at once. Take a look at this diff for inspiration. Deriving the key is fine with me.
  2. Figure out a way to not include that "REACT_INTERNAL_ROOT", I don't think it's needed and it's a bit ugly :) I think the path could start at it's own id.
diff --git a/packages/react-arborist/src/data/create-root.ts b/packages/react-arborist/src/data/create-root.ts
index ca567b3..7db17cd 100644
--- a/packages/react-arborist/src/data/create-root.ts
+++ b/packages/react-arborist/src/data/create-root.ts
@@ -11,12 +11,14 @@ export function createRoot<T>(tree: TreeApi<T>): NodeApi<T> {
     parent: NodeApi<T> | null
   ) {
     const id = tree.accessId(data);
+    const path = parent ? [...parent.path, id] : [id];
     const node = new NodeApi<T>({
       tree,
       data,
       level,
       parent,
       id,
+      path,
       children: null,
       isDraggable: tree.isDraggable(data),
       rowIndex: null,
diff --git a/packages/react-arborist/src/interfaces/node-api.ts b/packages/react-arborist/src/interfaces/node-api.ts
index 34d7138..7a75840 100644
--- a/packages/react-arborist/src/interfaces/node-api.ts
+++ b/packages/react-arborist/src/interfaces/node-api.ts
@@ -12,6 +12,7 @@ type Params<T> = {
   isDraggable: boolean;
   rowIndex: number | null;
   tree: TreeApi<T>;
+  path: string[];
 };
 
 export class NodeApi<T = any> {
@@ -23,10 +24,12 @@ export class NodeApi<T = any> {
   parent: NodeApi<T> | null;
   isDraggable: boolean;
   rowIndex: number | null;
+  path: string[];
 
   constructor(params: Params<T>) {
     this.tree = params.tree;
     this.id = params.id;
+    this.path = params.path;
     this.data = params.data;
     this.level = params.level;
     this.children = params.children;

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