Add DnD feature to TreeView #4207
Comments
matuzalemsteles
added
rfc
matuzalemsteles
added this to the LPS-133328 - Design and implement scalability improvements in the TreeView component milestone
|
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 Concerning the implementation itself, since we'll need to reorder the |
|
We talked a few things about this via call on Slack, I'll just add a few things.
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 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.
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.
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 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 |
|
@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 |
|
@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 Which seems very strange, since it's the first time I'm seeing this happening. I haven't changed my |
|
@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 [EDIT 2]: Finally out of the loop. What I did was checkout all my temporary changes, uninstall |
|
@matuzalemsteles just an update, using |
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 😅.
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. |
Switching back to
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 |
Yes it might make sense, now as I commented before in the expandable PR, I think we can ignore the reordering and 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 Regarding generating a uuid for each item, maybe it would not be necessary because for dynamic content it usually has |
Are you saying that for "static" content you'd consider not adding drag and drop support? I also think we need to re-think the "inner components" a bit, because in my opinion this is too complex, we have
I'm surprised you mention coordinates: in the drag and drop libraries I've played with ( |
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
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 The To change Would 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 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 {(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
Yeah we still need Actually looking at your example now, I think that's what I'm talking about here is, in this case the 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);
}; |
|
@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 What we could do, maybe add the 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. |
|
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 |
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.
I don't know if I'm over simplifying this in my head, but I see it like this (pseudo code) 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 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 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. |
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
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 <TreeView onDrag={(...) => }>
{...}
</TreeView>
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. |
|
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. |
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
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.
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. |
Yes it does.
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.
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) 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.
In this case
|
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
haha yeah 😂,but yes i understand it can be complex in your opinion, let's improve this.
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.
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.
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 |
|
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 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. |
|
Sorry closed by mistake |
|
This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-133328 |
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.
The text was updated successfully, but these errors were encountered: