Add DnD feature to TreeView

[matuzalemsteles]

The TreeView specification puts the DnD implementation as optional, we just have to study if this is feasible to be added to a TreeView and the implications that this can bring in terms of performance.

• DnD Spec

[julien]

Hey @matuzalemsteles, the DnD Spec doesn't really specify anything: it's just a screenshot with the sentence "Dropping a node on a valid target". I understand that we want to be able to drag one node onto another, but do you think we need to speak with lexicon to make sure we're on the same track?

One the technical side of things:

I noticed we already have react-dnd in the project (can't link to the yarn.lock file because GitHub won't show a preview but you'll find it there), even though it's only used for examples and wanted to know if you thought it would be a good idea to keep on using it. I'm asking because we use a rather old version (10.0.2), and upgrading to the latest isn't an option right now because it requires node 12.x (even thought the package.json in their repo says it's ok with 10.x).

Concerning the implementation itself, since we'll need to reorder the TreeView's items when the drag operation ends, I was thinking of using the id of the dragged item as well as the id of the item it was dropped on in order to achieve this. The only problem I'm facing is that the TreeViewItem is aware of it's id (via useItem) but the parent TreeView has no idea about this, which is kind of strange (shouldn't the item's ids be generated straight away).

[matuzalemsteles]

We talked a few things about this via call on Slack, I'll just add a few things.

the DnD Spec doesn't really specify anything: it's just a screenshot with the sentence "Dropping a node on a valid target". I understand that we want to be able to drag one node onto another, but do you think we need to speak with lexicon to make sure we're on the same track?

We agree that we are going to talk to Lexicon to understand the requirements, Julien raised some interesting points as it may exist in some cases that have a tree based on files where we have documents and images, sometimes these cases do not make sense to move a document to become a sibling of the folder, this would be a case-by-case drop rule. But the important thing is whether we should also allow only reordering nodes on the same level or would it be allowed to move up and down.

cc @drakonux @hold-shift

As this would also be optional and not required, we can also ignore this for now until we understand the requirements more. If for this case.

I noticed we already have react-dnd in the project (can't link to the yarn.lock file because GitHub won't show a preview but you'll find it there), even though it's only used for examples and wanted to know if you thought it would be a good idea to keep on using it.

Oh yeah, I think we added it to the demos just to demonstrate how some Clay components using React DnD, the biggest motivation is that DnD could be implemented in different ways by the team, usually, each one had its own visual and interaction strategy.

Anyway, we agree to try using another library, like react-beautiful-dnd.

Concerning the implementation itself, since we'll need to reorder the TreeView's items when the drag operation ends, I was thinking of using the id of the dragged item as well as the id of the item it was dropped on in order to achieve this. The only problem I'm facing is that the TreeViewItem is aware of it's id (via useItem) but the parent TreeView has no idea about this, which is kind of strange (shouldn't the item's ids be generated straight away).

I think we've talked about it but anyway the current implementation is that it works for the different cases, the static and dynamic content, so the TreeView component will use the items for dynamic content and will create an Array to from the children when it is static content. The point I want to make is that we use the ItemContextProvider nested, so it's declared so that rendering structure level components have visibility of the item being rendered at that moment, it would be the same thing to think about it with rendering in tree, so the TreeView component has no visibility because it starts rendering the items, so it will only have visibility of the items.

But I think I understand your point the difference is that we didn't use a flat tree structure to render the elements because I didn't want to build this structure before starting rendering and it would be difficult to keep it up to date, for example, any change that occurs in this structure would have to be reflected in the items structure, if we move an element we would have to do the same in the items structure, one thing we can investigate is to allow rendering using flatten structure so we can rely on that structure to handle this without creating it in runtime but would still have to investigate how to do this with a user-friendly design. Apparently, some APIs already support requesting the flat structure, my concern is that this is not true for everything.

[julien]

@matuzalemsteles thanks for adding this information here.

I wanted to add one more thing about the spec, I briefly talked with @drakonux on Slack and we agreed that the main use case is to be able to drag nodes around, ideally expanding them if they have nested nodes (like directories) when they are "hovered". Concerning replacing nodes, I think this can be done in the future and we might want to leave this as something to be implemented by the developer, maybe a callback, although that part is still not clear for me in terms of implementation - so that's one more thing we can analyze.

I'm also going to use react-beautiful-dnd (interestingly it also uses render props to allow rendering "anything"), it's a bit more straightforward to use than react-dnd but we'll still need to be able to manage the items based on ids of some sort, so I'll grab the latest changes you made to #4222 and have a thought on this (since now we're using keys and not ids).

[julien]

@matuzalemsteles just putting this here in case in rings a bell while I try to find a solution.

I fetched #4222 to start working on dnd support. Trying to add any package from the clay-core module with yarn add DEPENDENCY_NAME fails with the following message.

error An unexpected error occurred: "expected workspace package to exist for \"eslint\"".

Which seems very strange, since it's the first time I'm seeing this happening. I haven't changed my node, npm or yarn version.

[julien]

@matuzalemsteles follow up on my previous comment, the only way I managed to fix was with this solution, probably not something we want to do, but at least I'm not stuck anymore.This reminds me how broken frontend tooling is.

[EDIT 1]: Actually didn't fix everything because yarn updated the react-dnd version so now our drag and drop demos are broken ... I have the feeling I'm going to be stuck in this loop for a while. I'll undo my changes, and see what happens.

[EDIT 2]: Finally out of the loop. What I did was checkout all my temporary changes, uninstall yarn and re-install specifically version 1.19.0, re ran yarn install DEPENDENCY_NAME and no complaints this time ... so I'll stick with that version for now. I'm surprised this even happened.

[julien]

@matuzalemsteles just an update, using react-beautiful-dnd with the current implementation of the TreeView as it is right now in #4222 works but it's very tricky, maybe because of the fact it's both using dynamic and static content or because of the render props usage, or the way the components are structured. I think we should take time to think about this a bit before thinking of adding more features, because if adding drag and drop is costing so much, it might be a sign that this is too complex. (Just a thought) but that's also related to what you were asking concerning #4218

[matuzalemsteles]

Trying to add any package from the clay-core module with yarn add DEPENDENCY_NAME fails with the following message.

It pops up to me sometimes too, but I don't remember now how I fix it. Apparently, it's related to the yarn version and they fix it in v2 😅.

just an update, using react-beautiful-dnd with the current implementation of the TreeView as it is right now in #4222 works but it's very tricky, maybe because of the fact it's both using dynamic and static content or because of the render props usage, or the way the components are structured. I think we should take time to think about this a bit before thinking of adding more features, because if adding drag and drop is costing so much, it might be a sign that this is too complex. (Just a thought) but that's also related to what you were asking concerning #4218

Hmm I see, I'm thinking of a way to use rendering with a flat structure but for that I only see the option to support flat structure data to not do this at runtime but I can't see a way to support static content. I only see the option to change the tree structure and traverse to make the drop changes.

If we are going to make a flat tree structure at runtime, we will have trouble keeping it up to date, because we will also have to update the tree anyway.

[julien]

It pops up to me sometimes too, but I don't remember now how I fix it. Apparently, it's related to the yarn version and they fix it in v2 sweat_smile.

Switching back to 1.19.0 made it work for me, seems like it's fixed for now.

Hmm I see, I'm thinking of a way to use rendering with a flat structure but for that I only see the option to support flat structure data to not do this at runtime but I can't see a way to support static content. I only see the option to change the tree structure and traverse to make the drop changes.

If we are going to make a flat tree structure at runtime, we will have trouble keeping it up to date, because we will also have to update the tree anyway.

I agree, it's not easy. I also think we could generate an "id" for each child when the component is created, for example, when we receive the items prop, we could generate a "uuid" for each child and that way we'd have a way to know which item has been moved after a drag and drop and reorder our "items" array. Does that make sense?

[matuzalemsteles]

I agree, it's not easy. I also think we could generate an "id" for each child when the component is created, for example, when we receive the items prop, we could generate a "uuid" for each child and that way we'd have a way to know which item has been moved after a drag and drop and reorder our "items" array. Does that make sense?

Yes it might make sense, now as I commented before in the expandable PR, I think we can ignore the reordering and drop for static content because even though we build a flat structure in parallel we wouldn't be able to use it to change and have effect on rendering of the elements. Because we would need to build the flat structure before rendering and lock the rendering until it's done (the lock isn't necessarily explicit) so we could use the structure for rendering and any effect it has will affect it visually.

The problem with this is that it is not very cheap, mainly because we are concerned with performance and we would also have to create a synchronization for everything that is modified in the flat structure must also be reflected in the tree structure, for example:

const flatItems = {
  main: {
     children: ['1']
  },
  '1': {
     children: ['2', '3'],
     parent: 'main'
   },
  '2': {
     children: ['4', '5', '7'],
     parent: '1'
   },
};

const treeItems = [
  {
    children: [
      {
        children: [
          {children: [...], id: '2', name: 'Child 2'}
        ],
        id: '1',
        name: 'Child 1',
      }
    ],
    id: 'main',
    name: 'Main'
  }
];

// For example, mover the Node 2 to be Node 1 sibling:

moveToSibling('2', 'main');

// Result
const flatItems = {
  main: {
     children: ['1', '2']
  },
  '1': {
     children: ['3'],
     parent: 'main'
   },
  '2': {
     children: ['4', '5', '7'],
     parent: 'main'
   },
};

const treeItems = [
  {
    children: [
      {
        children: [...],
        id: '1',
        name: 'Child 1',
      },
      {children: [...], id: '2', name: 'Child 2'}
    ],
    id: 'main'
    name: 'Main'
  }
];

Because once we receive the items in the tree structure whether static or dynamic and we create an internal flat structure we need to reflect on the tree structure to return the changes so it can be saved, so I keep thinking that in this case instead of using the structure flat to move the nodes, we could use the tree directly, unless we find a way to simplify a modification to the tree using the flat structure that doesn't cost much, I have no idea how we could do that but we can explore.

Another thought about this is just accepting flat structures but I don't know how that would work in a composition environment that uses a tree structure, that way we wouldn't worry about modifying the tree since we'll have the flat structure but the problem with that is that we can have a lot of cons, maybe it doesn't work for static content or composition normally.

A second thought is perhaps to see a way that the flat structure helps us to reflect changes in the tree structure, perhaps during rendering of the <Collection/> component we can pass coordinates to the item, for example, the item level in the structure or something that helps us quickly access the item in the tree and move to another level, probably there should be some algorithm that helps us with this, I'll research more about it.

Regarding generating a uuid for each item, maybe it would not be necessary because for dynamic content it usually has id and as we need key to be declared explicitly or implicitly we have key as uuid inside the tree but if a tree is modified and the key has been generated that doesn't help much, but I think for dynamic content we can trust it to have id, at least looking at the structures we have in DXP it looks ok because we would have to know a way to preserve the uuid and know when to revalidate and that would be more problematic.

[julien]

Yes it might make sense, now as I commented before in the expandable PR, I think we can ignore the reordering and drop for static content because even though we build a flat structure in parallel we wouldn't be able to use it to change and have effect on rendering of the elements

Are you saying that for "static" content you'd consider not adding drag and drop support? Maybe we're making this component too "smart", why do we need to deal with both dynamic and static content, couldn't we just settle on one way and see how it goes? I think that having an API that is too permissive is sometimes problematic because there are many ways to use the component and a lot of use cases to handle.

I also think we need to re-think the "inner components" a bit, because in my opinion this is too complex, we have TreeView, Collection, TreeViewGroup, TreeViewItem and TreeViewItemStack, couldn't we just have something like TreeView, TreeViewBranch and TreeViewNode?

A second thought is perhaps to see a way that the flat structure helps us to reflect changes in the tree structure, perhaps during rendering of the component we can pass coordinates to the item, for example, the item level in the structure or something that helps us quickly access the item in the tree and move to another level, probably there should be some algorithm that helps us with this, I'll research more about it.

I'm surprised you mention coordinates: in the drag and drop libraries I've played with (react-dnd and react-beautiful-dnd) you typically use "ids" (think uuids or indices) to know which item was dragged and dropped, as I said, I'm no expert when it comes to React, but I'd rather think about my items in terms of "data" than "dom elements", I don't want to deal with coordinates, I just want to reorder the tree structure once an item has been dropped at which point the component will be re-rendered. As an example, please take a look at this codesandbox link and tell me what you think.

[matuzalemsteles]

Maybe we're making this component too "smart", why do we need to deal with both dynamic and static content, couldn't we just settle on one way and see how it goes? I think that having an API that is too permissive is sometimes problematic because there are many ways to use the component and a lot of use cases to handle.

Yeah, as it's a bit smart we abstracted a lot of the complexity into the component so that the component is easy to use, we can manage it anyway I think if the component is easy to use we can handle it well. Yes, the idea is just to support DnD for dynamic content, I think static content is more difficult to deal with because the structure we would have to change to change the rendering would be the children and it would give more problems and more complexity that I think we can ignore now.

I also think we need to re-think the "inner components" a bit, because in my opinion this is too complex, we have TreeView, Collection, TreeViewGroup, TreeViewItem and TreeViewItemStack, couldn't we just have something like TreeView, TreeViewBranch and TreeViewNode?

Hmm, I wanted to change some but it's quite complicated due to the current markup structure which makes it a bit more complicated, for example, I didn't want to have ItemStack and Item, but due to the way the markup is done, I needed one component as support to help create Group, I can abstract this component internally sometimes but when you have a Group it becomes necessary to differentiate what is part of the Group and what corresponds to the Node or Stack like the spec refers to the Node's content.

The Collection component is just an abstraction of the logic that is used in the Group and the TreeView, so we just need it to not replicate code.

To change TreeViewItem to TreeViewNode I'm not sure, there's one thing I wish we did more is maintain consistency between Clay components (https://github.com/liferay/clay/milestone/47) and The term Item is more common to be adopted for all other components. We still don't do it for all components but we use similar terms, this also seems more common for collections. What do you think? does it make sense?

Would TreeViewGroup become TreeViewBranch? Well I think Group is more common 😅, I would be lost if I see a Branch if it's related to creating groups for the Node.

I think overall, we have far fewer components than we would have if the component wasn't abstracting some use cases, especially compared to other components that we have, I think we're fine with this one. For example, we are very markup oriented to create the components, so instead of always following the markup which could generate many other components like TreeViewAction (this maybe we still have to have, I still have to validate this case), TreeViewIcon, TreeViewText... we can abstract some of these components internally.

One point why I like render props to make the component more flexible is because we have context-based APIs, for example, having the component TreeViewItem, TreeViewItemStack and TreeViewGroup are what builds Node, if you see the case for Page Elements in Page Editor or other use cases that have in the specification, we would have to change the markup to support TreeView with buttons, adjust the markup to support Sticker to list a hierarchy of users, with this API we have a smooth abstraction to touch components to modifying the markup structure, when I refer to context-based APIs I'm talking about having APIs for each of these components instead of concentrating APIs scoped in a main component, that's the other discussion but we can have this for example:

{(item) => (
    <TreeView.Item as={PageElements.Item}>
        <TreeView.ItemStack as={PageElements.Stack}>
            <Icon symbol="folder" />
            {item.name}
        </TreeView.ItemStack>
        <TreeView.Group items={item.children}>
            {(item) => (
                <TreeView.Item>{item.name}</TreeView.Item>
            )}
        </TreeView.Group>
    </TreeView.Item>
)}

In other words, we can easily modify the components without having an intermediate component that sends the props to the internal component and the composition seems so natural to me. So as as an API, is under the context of each component, so we implicitly have all the APIs available.

I'm surprised you mention coordinates: in the drag and drop libraries I've played with (react-dnd and react-beautiful-dnd) you typically use "ids" (think uuids or indices) to know which item was dragged and dropped, as I said, I'm no expert when it comes to React, but I'd rather think about my items in terms of "data" than "dom elements", I don't want to deal with coordinates, I just want to reorder the tree structure once an item has been dropped at which point the component will be re-rendered. As an example, please take a look at this codesandbox link and tell me what you think.

Yeah we still need id or uuids, when I refer to coordinates it is to know where this item is in the items tree, sorry adding dom elements to the conversation is more confusing, but for example react- dnd and react-beautiful-dnd let's use id in their examples, they use a lot of list, so that's why they change the list it's easier, in our context we would have to change the tree structure, so the coordinates are in relation to the index that the item is in the tree to allow accessing O(1) instead of traversing the tree to know where the item is and where to move to, ie two operations, delete and add, depending on the size of the tree this would be extremely expensive, I suffered cases like this in Forms, where the traverse of the tree is abused a lot to modify the data.

Actually looking at your example now, I think that's what I'm talking about here is, in this case the level you would be the coordinate I'm referring to. The difference is that in onDragEnd we would do the operation to remove the sourceItem from items and we would add it to the tree in another position, basically that's what you are already partially doing, maybe we don't need findById for example, we would have something like deleteItem and addItem with properties passing the coordinates to make the change O(1) otherwise we would have to search recursively, another problem to be careful about is that changing the destinationItem directly is that every time we do the Drop we must have a copy of the items to handle it as immutable, otherwise changing structure values causes a lot of side effects in rendering even after updating. Considering the pseudocode of your example.

const onDragEnd = (result) => {
    // Some stuff...

    const copyData = createImmutableData(data);

    // We can also use the ID without problems but imagine the case of an item
    // deep in the tree, then we will have to search for this element recursively
    // what would be `O(n!)` in the worst case which is much more likely.
    const sourceItem = deleteItem(copyData, id);

    // Using indices that we can match on each recursive rendering.
    // This would be `O(1)` because each element in the array is the
    // element's render index.
    const sourceItem = deleteItem(copyData, [0, 0, 0, 1, 2]);

    // We can also use the index to know where to add the element. In a
    // reordering, we need to know in which position we should add the
    // element, so the indices already cover that case.
    addItem(copyData, sourceItem, [0, 0, 2]);

    setData(copyData);
  };

[matuzalemsteles]

@julien regarding DnD I've been thinking about this a bit to see how it would be used taking into account the DXP use case and I realized that for the Page Editor, they implement their own DnD mechanism because the TreeView doesn't have it either, but looking there are some things that maybe we can make their life easier and maybe ours, for example: When dropping the node on some other node in onDragEnd I'm really not sure now if we should actually move or manipulate the items structure, for the Page Editor case they create the tree structure from a flat structure and when you move a node they manipulate the flat structure which in the end will recreate the items structure, this is quite costly.

What we could do, maybe add the onDragEnd API to the TreeView that we can call anywhere in the component and let the developer handle manipulating the data but we can also add an onDragEnd by default that handles the items' structure. Just thinking about it, they go around to having the tree structure and if we manipulate the items internally they will get the updated items and will have to convert these items to the flat structure to update 😅, it looks like a circle, get flat items from backend -> create tree -> treeview -> drop -> update tree -> convert to flat items ->....

A thought is also how we could support the flat structure without having to increase the complexity of our component and also for those who use it. I'm going to start exploring something along these lines.

[matuzalemsteles]

Well I did some testing quickly and it looks like we don't need to implement a lot to support flat structure in items, so this might decrease the complexity for the Page Editor case and we can recommend its use more. I didn't need to do much to implement it, most of the rule was in the <Collection/> component, the only problem is that I can't do the strong typing so that the item of a Group has the type correctly. See this commit https://github.com/liferay/clay/pull/4222/commits/19e2816dc7f1300d32f68c7e3e61eab586a01a55, I also added a demo to Storybook. Let me know what you think.

[julien]

Yes, the idea is just to support DnD for dynamic content, I think static content is more difficult to deal with because the structure we would have to change to change the rendering would be the children and it would give more problems and more complexity that I think we can ignore now.

Sorry if I'm repeating myself, but as I said in #4218, making features available under certain conditions feels wrong to me. On top of that I think that even if it's nice to be able to render both static or dynamic content, I don't think the tree is going to be used with static content.

Hmm, I wanted to change some but it's quite complicated due to the current markup structure which makes it a bit more complicated, for example, I didn't want to have ItemStack and Item, but due to the way the markup is done, I needed one component as support to help create Group, I can abstract this component internally sometimes but when you have a Group it becomes necessary to differentiate what is part of the Group and what corresponds to the Node or Stack like the spec refers to the Node's content.

I don't know if I'm over simplifying this in my head, but I see it like this (pseudo code)

TreeView
    TreeItem
         (If the item has children) TreeItem
               etc...

Concerning the component's name, I'm totally fine with using the word Item, I used the word Node, because it's commonly used when talking about trees or tree traversal.

If you think render props is the way to go, I'm OK with that as long as it's the "only option", if it can be avoided it would be even better (but we already talked about that in #4218), so I'll wait for the conclusion on that thread.

Concerning my "example", it was just a test, and to make it easier we need to use a Tree like data structure for our items, and use tree traversal algorithms to make it easy to work with when (so as you said, in that case they're would be no need for findById - or at least not like it is currently implemented).

Anyway, until #4218 is not merged, it's going to be hard implementing drag and drop; but I still wanted to add one more point: it seems that you are mixing "dragging and dropping items in the tree" (i.e. reordering the nodes in the tree) with "dragging and dropping items from the tree" (i.e. what's done in the page editor). ç

I'm only talking about the first case, and for the page editor, the team will need to implement that - we obviously need to give to provide the correct API (for them to know which item has been dropped, etc...) but it's not our goal to implement that feature, I'm 100% sure about that.

The last thing I wanted to say (again, sorry for repeating myself), is that even though the flat data structure example you added in https://github.com/liferay/clay/commit/19e2816dc7f1300d32f68c7e3e61eab586a01a55 makes it much easier to work with the items, we now have another way of specifying the items and that's something I would like to avoid (I added more details in #4218). Before moving on, let's focus on #4218 if you agree.

[matuzalemsteles]

Sorry if I'm repeating myself, but as I said in #4218, making features available under certain conditions feels wrong to me. On top of that I think that even if it's nice to be able to render both static or dynamic content, I don't think the tree is going to be used with static content.

Well, we already do that for low-level and high-level if you want features like dynamic rendering and other implementation details you should use high-level I would say implementing DnD for static would be a temporary resource limitation, as I said we can be consistent and keep the same for everyone but to add to static requires more work, in my view it's not worth adding to static now, I've commented on this in other threads now but static is important because it's the low implementation level -level and static can cover cases like the implementation for Vertical Navigation.

So static here is cheap to maintain what I would recommend is not to worry about it, if you see in the implementation the Collection is quite explicit when dealing with static and dynamic, so it wouldn't be a problem to just add the DnD feature to dynamic.

Anyway, until #4218 is not merged, it's going to be hard implementing drag and drop; but I still wanted to add one more point: it seems that you are mixing "dragging and dropping items in the tree" (i.e. reordering the nodes in the tree) with "dragging and dropping items from the tree" (i.e. what's done in the page editor). ç

Well, I think it got more confusing haha, but DnD and independent reordering are related to drop 🤪, so they are the same thing, you use DnD to use reordering if there is a rule to say you will drop above or below, that would another implementation detail.

In the end, anyway, if we don't need to deal with the drop case we can just expose the onDrag method in the TreeView as I mentioned and we don't deal with the "move" or any rules whatsoever. Does it make sense to you?

<TreeView onDrag={(...) => }>
  {...}
</TreeView>

The last thing I wanted to say (again, sorry for repeating myself), is that even though the flat data structure example you added in 19e2816 makes it much easier to work with the items, we now have another way of specifying the items and that's something I would like to avoid (I added more details in #4218). Before moving on, let's focus on #4218 if you agree.

Yes, the good part is that we are agnostic so it wouldn't be a problem, we dealt with it but there is no overhead and it not impacts other resources, especially if we can't deal with the drop, which sounds great to me. But we can discuss this after we reach a consensus on #4218.

[julien]

Hey @matuzalemsteles I don't agree that having static content and dynamic content makes sense if they behave differently. It's not so cheap to maintain either because all the complexity (and there's a lot) is hidden in the component - and how can I not worry about it.

I probably didn't explain it very well but, when I talk about drag and drop, I mean reordering items in the TreeView, not dropping items from the TreeView in the Page Editor - I think this might help understand.

[matuzalemsteles]

I probably didn't explain it very well but, when I talk about drag and drop, I mean reordering items in the TreeView, not dropping items from the TreeView in the Page Editor - I think this might help understand.

I understand it's dragging inside the TreeView 😉 What I'm saying is that in the case of the Page Editor, they manipulate your data structure which is in the flat structure, and then create the tree structure to pass it to the TreeView, this is costly, for example, see this code example: https://github.com/liferay/liferay-portal/blob/d57aa80761a81eb279776362bea81012481b3b7e/modules/apps/layout/layout-content-page-editor-web/src/main/resources/META-INF/resources/page_editor/plugins/browser/components/page-structure/components/StructureTree.js#L117

When I was referring to circulation, do we manipulate the tree structure we have to call a callback so that the superior component can update its state right? in this case of the Page Editor they just have a flat structure, which means for them to update that flat structure it would be an overhead of converting the tree structure to flat. Does it make sense to you?

Also, this could be avoided anyway, for example even updating the tree structure and keeping the state in the upper component ie controlled, once we expose the onDrag the team doesn't need to deal with converting the tree structure to the structure flat to update, in this case, They could get the information from onDrag and update the structure but the problem with that is that we would have two states being manipulated and that it is very expensive but it would be a viable option.

I don't agree that having static content and dynamic content makes sense if they behave differently.

Hmm, well how are we going to implement for static? or better how would this be implemented in a low-level component? the static is actually just a reflection of a low-level component. If you look at the other components we have this is our default, low-level components don't implement all features but high-level implement all features. In this case we have 2 in 1. In fact, the static just doesn't support DnD.

It's not so cheap to maintain either because all the complexity (and there's a lot) is hidden in the component - and how can I not worry about it.

Hmm, very hard to say what is complex, complex may be different for each person but I still believe the component is very simple in fact it is very simple, if you look at the implementations of other libraries you will probably see that we are actually a humble library with few resources 😕.

So that's exactly what I want to do with the TreeView, it will be simple to use without having to configure a lot of things or having to do your own implementation, the cost for this is to abstract inside the component and reduce the complexity outside.

Well anyway, what do you see that's complex? show some code examples so we can improve.

[julien]

I understand it's dragging inside the TreeView wink What I'm saying is that in the case of the Page Editor, they manipulate your data structure which is in the flat structure, and then create the tree structure to pass it to the TreeView, this is costly, for example, see this code example: https://github.com/liferay/liferay-portal/blob/d57aa80761a81eb279776362bea81012481b3b7e/modules/apps/layout/layout-content-page-editor-web/src/main/resources/META-INF/resources/page_editor/plugins/browser/components/page-structure/components/StructureTree.js#L117

When I was referring to circulation, do we manipulate the tree structure we have to call a callback so that the superior component can update its state right? in this case of the Page Editor they just have a flat structure, which means for them to update that flat structure it would be an overhead of converting the tree structure to flat. Does it make sense to you?

Yes it does.

Hmm, well how are we going to implement for static? or better how would this be implemented in a low-level component? the static is actually just a reflection of a low-level component. If you look at the other components we have this is our default, low-level components don't implement all features but high-level implement all features. In this case we have 2 in 1. In fact, the static just doesn't support DnD.

I know that here with TreeView we're trying to combine low-level and high-level components, but what I'm saying is that I find it harder to maintain, and we also have to do constant checks (and that does introduce complexity at some degree) to see if we're actually using static or dynamic content to enable drop and drop support (for example). I just find it strange that when using static content you don't get all the features. For example, with ClayButton and ClayButtonWithIcon, I don't see why ClayButton couldn't render a ClayIcon depending on the props it receives (and I think there are other similar components in clay)

complex may be different for each person but I still believe the component is very simple in fact it is very simple

That's a pretty philosophical comment :smile: And you are right, but that's your opinion and not necessarily what everyone thinks (yes that includes myself)

For someone who has a lot of experience with React (and TypeScript) everything might seem simple, but if you were to jump into Clay's codebase today without all that experience I doubt you'd find it "simple". In fact, as an example, product teams often open issues, but have you noticed that they don't necessarily send a fix, I think it's because they might not be able to.

Well anyway, what do you see that's complex? show some code examples so we can improve.

In this case

• The fact that the component has to deal with static and dynamic content and that based on that some features will or won't be available (for the moment with multiple selection you didn't have to deal with that, but for DND it's the case)

• Mostly what's in useMultipleSelection.tsx

  • Too many hooks

[matuzalemsteles]

I know that here with TreeView we're trying to combine low-level and high-level components, but what I'm saying is that I find it harder to maintain, and we also have to do constant checks (and that does introduce complexity at some degree) to see if we're actually using static or dynamic content to enable drop and drop support (for example). I just find it strange that when using static content you don't get all the features.

Yeah, I see, maybe the problem is how to organize the DnD in all this, well I think we can continue like this and have the PR of DnD and see how the degree of complexity, will be like I said to add DnD to the static but this it would take more work anyway, which I don't think it's worth it because if you're using static it means the developer has simpler cases like a Vertical Navigation.

Or it can be totally different also see DropDown, with WithItems the static cases with the low-level are actually the most used to make the dynamic.

That's a pretty philosophical comment 😄 And you are right, but that's your opinion and not necessarily what everyone thinks (yes that includes myself)

haha yeah 😂,but yes i understand it can be complex in your opinion, let's improve this.

For someone who has a lot of experience with React (and TypeScript) everything might seem simple, but if you were to jump into Clay's codebase today without all that experience I doubt you'd find it "simple". In fact, as an example, product teams often open issues, but have you noticed that they don't necessarily send a fix, I think it's because they might not be able to.

It's normal that teams don't send corrections because they don't know the products, we have some people that send once but it's not common, I think it's not tied to complexity Clay is very simple in terms of components if you look at the bases of DXP code with React, for sure it's much more complex than that, so it's an issue that they delegate the problem to the team instead of fixing, that's another, cultural one.

The fact that the component has to deal with static and dynamic content and that based on that some features will or won't be available (for the moment with multiple selection you didn't have to deal with that, but for DND it's the case)

We can see how this will be reflected in the code, I know you're already seeing issues with this, but I need to look at this as well and see what can be improved, so before making the decision to remove static we need to check if it's by a big reason, I don't see a problem with retaining internal complexity if the component is to be useful, easy... but that would be another discussion, my proposal is: let's continue with this and when we have an idea of DnD ready for review, we can come back with this conversation, too I can help you with that if you want.

Mostly what's in useMultipleSelection.tsx Too many hooks

Hmm, are you finding this complex? is there something we can improve? We can decrease the amount of hooks, I think I can move what's in useMultipleSelection to useTree anyway. What do you think? But we need these codes regardless if it doesn't have static, or render props for example.

[julien]

Hey @matuzalemsteles I submitted a draft pull request in #4253.

You'll see that the drag and drop is mostly working, but I couldn't figure out why sometimes the TreeView does not re-render when dropping a node. I finally settled to use react-dnd because it was already being used as a dependency and that react-beautiful-dnd doesn't really bring anything else.

You'll also notice some code in order to generate indices for each node, this is mostly what I've used to be able to reorder items (and I didn't really find another way to do this), and I think you might want to refactor that part the "react" way, or if you have any tips for me please let me know.

[julien]

Sorry closed by mistake

[github-actions[bot]]

This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-133328