feat(@clayui/core): adds functionality for expandable nodes and nested nodes

[matuzalemsteles]

Fixes #4210

This functionality is written under PR #4218. Still putting it as a draft because it's not done, I still need to tweak the code and clean up some parts.

As I mentioned in PR, it is possible to make expandable without necessarily having to render props, but that would need a restricted API, as we commonly use in high-level.

Why not use maps actually people can use them if they want, but the component won't have visibility of items that are rendered just from the markup but we can't do much without the control we can have with render props, neither would we be able to do nested rendering automatically or apply virtualization strategies, perhaps even a scroll-based "load more", commonly known as an infinite scroll.

<TreeView>
      {ITEMS.map(item, () => (
        <TreeView.Item>
          {item.name}
          <TreeView.Group>
            {item.children.map(item, () => (
              <TreeView.Item>{item.name}</TreeView.Item>
            ))}
          </TreeView.Group>
        </TreeView.Item>
      ))}
</TreeView>

For an example of usage, see the storybook with the case of nested. As I mentioned in the previous PR, we can do this in different ways but the purpose here is also to find a middle ground so that we don't need to create a low-level TreeView without the features and a high-level with the features and an API restrict as we do for the other components.

[matuzalemsteles]

I sent a few more commits, cleaning up some more code and simplifying rendering render props to better handle Expandable Nodes, Expandable Nodes work OOTB for dynamic or static content. Behaviors can be configured manually or dynamically, for example:

• Dynamic content where the data contains the id property is used as the element's key and identifier to make the Node expandable toggle
• Dynamic content without an id is considered the key generated by React or index
• Element without key is considered the key generated by React
• Element with key is used as the identifier

{(item)} => (
  <TreeView.Item key={item.uuid} />
)}

{(item)} => (
  <TreeView.Item />
)}

{(item)} => (
  <TreeView.Item key={item.name} />
)}

<TreeView.Item key="foo" />
<TreeView.Item />

The idea is to allow the possibility of configuring what is the unique identifier of the element to deal with expandable nodes and multiple selection, this makes it easier to define which nodes are expanded or selected as soon as the component is assembled, for example.

const [expandedKeys, setExpandedKeys] = useState<Set<React.Key>>(
  new Set(['Foo', 'Bar'])
);

<TreeView.Item key="Foo">
  Foo
  <TreeView.Group>
    <TreeView.Item key="Bar">
      Bar
      {...}
     </TreeView.Item>
  </TreeView.Group>
</TreeView.Item>

I also added one more example to demonstrate PageEditor's TreeView use case that lists the Pages Elements, this is not 100% faithful to the implementation because the appearance of the elements, hover are totally different but I think it would be worth building one faithfully to demonstrate the ergonomics of the API. As a consequence I added the expanderIcons API to customize the Expander icons, this API is still not final, so it may be that the API type changes to be more ergonomic.

[matuzalemsteles]

Well now this looks more polished and implements multiple selection for the TreeView as per the Tree View spec and the expected states when selecting a Node that has many children and adding intermediate state for parents.

I added some comments in the code so it helps to understand some motivations of how it was done, as we want the TreeView to be the fastest, I went with some different strategies to reduce the tree traversal to make multiple selection, in the end, it was a good result, interesting.

As the TreeView can be used statically or dynamically, we cannot use the state as well as the expandable implementation that distances from the data, to implement the selection for two types of content, we need to create a flat tree instead of a nested tree for To lower the traversing cost, the key is the important factor I used to create this because it becomes unique when it is rendered in-depth when useItem() was implemented.

The creation of the flat tree is built on top of React's rendering stream, ie we don't need to create a separate stream to update this is much more optimized than taking the items structure and creating a tree flat from it and it would be much more costly to update because we would have to traverse the tree every time to update, this way is even more optimized because we can specifically remove the unmounted element from the structure, removing the item and removing the reference in the parent, we also don't need to recursively remove the children because React will do the rest.

• Demo

[matuzalemsteles]

The strategy to enable multiple selection is to add the ClayCheckbox component explicitly, we can also allow any component that has displayName that contains checkbox in the name, this allows for more customization.

One reason why I went with this is that it allows for explicit composition and the component is exposed so it also allows you to place the component anywhere in the stack.

I'm wondering if I do the same for the Expander because we have a case where the TreeView will replace the Vertical Navigation component and the Expander is on the right side instead of the left.

[julien]

we can also allow any component that has displayName that contains checkbox in the name, this allows for more customization.

Not a React or TypeScript expert, but if we're just relying on the displayName it doesn't make much sense to me. Another thing I don't understand is this React.cloneElement call ... I know it has to do with render props, but I don't find it easy to read.

[matuzalemsteles]

Not a React or TypeScript expert, but if we're just relying on the displayName it doesn't make much sense to me.

The displayName property is a React feature that lets you name the component name so it's easy to know what the component will be even when it's in production. For example, declaring displayName will be used in React DevTools to identify which component is being rendered. We could maybe use instanceof but we would only be allowing them to use ClayCheckbox for example.

The idea of relying on displayName is to help us identify which element is being used, for example see the other cases above this implementation. So this allows the developer to for example, add the Checkbox anywhere in the stack.

<TreeView.ItemStack>
  <Checkbox />
  <Icon symbol="folder" />
  Folder
</TreeView.ItemStack>

// or

<TreeView.ItemStack>
  <Icon symbol="folder" />
  Folder
  <Checkbox />
</TreeView.ItemStack>

In other words, anywhere in the stack. I also use this to abstract the extra markups that are added this allows to reduce the noise added by the markup and we also avoid creating new components.

Another thing I don't understand is this React.cloneElement call ... I know it has to do with render props, but I don't find it easy to read.

It's not actually related to rendering props using React.cloneElement, we use that to pass new properties to the element, for example this API allows to preserve properties that have already been passed to the component, you can read more about this https://reactjs.org/docs/react-api.html#cloneelement.

We can also use an alternative if it helps you understand better I don't see a problem either.

const Child = child as React.ReactElement;

content = (
	<Child.type
		{...Child.props}
		checked={selection.selectedKeys.has(item.key)}
		indeterminate={selection.isIntermediate(item.key)}
		onChange={() => selection.toggleSelection(item.key)}
	/>
);

[julien]

The displayName property is a React feature that lets you name the component name so it's easy to know what the component will be even when it's in production. For example, declaring displayName will be used in React DevTools to identify which component is being rendered. We could maybe use instanceof but we would only be allowing them to use ClayCheckbox for example.

At least, I knew that 😂 but what I meant is that instead of using the displayName it would be nice to have something a bit more robust (I'm not saying I know a better way to do it, but it's something that came to mind when seeing this part of the code), now with your example I see the benefit so just let's just ignore my initial comment and if we have the need to we can improve this later.

Thanks for the explanation concerning React.CloneElement, after having read the docs it (always) makes more sense.

[matuzalemsteles]

haha oh, I think I got it wrong 😅.

[julien]

No worries, that's what conversations are for.

[julien]

@matuzalemsteles I'll fetch this again to see if it clears out some doubts I have about #4207 .

I'll probably leave some comments about the questions I have since this have been changed since the last time I saw it (force push always confuses me on W.I.P. or drafts - having all the commits let you see what changed over time and generally why).

[julien]

@matuzalemsteles was this part of the ITreeViewProps before this change?
Can you explain why you moved it here?

[julien]

@matuzalemsteles why do we prefix interfaces with an I but we don't prefix (exported) types with a T and functions with an F? I know it's a common convention (I used to use the same in ActionScript back in the day) but since we don't use it consistently what's the added benefit?

[matuzalemsteles]

was this part of the ITreeViewProps before this change? Can you explain why you moved it here?

The implementation to handle rendering implements both sides, static and dynamic content, and was done in two components TreeView and TreeViewGroup, so they both had the same implementation all the time and it was starting to get duplicated code and I had to copy this all the time as well as the Props, so that's why I created a Collection that is used by both and we centralized and avoided duplicating code and types.

why do we prefix interfaces with an I but we don't prefix (exported) types with a T and functions with an F? I know it's a common convention (I used to use the same in ActionScript back in the day) but since we don't use it consistently what's the added benefit?

Actually, the goal is not to add a prefix to say what it would be, the I we added to avoid colliding names with functions, we don't do the same for type because we rarely use it, so it didn't need to, neither we like that but it happens name collision every time and we need to create a new different name one or add one more word it sometimes didn't make sense so we used the eslint rule just to cover this case if that wasn't the problem we certainly wouldn't be using it.

I wanted to use more of the typescript-cheatsheets patterns for React after they update they recommend using type but there's still a long discussion about that https://github.com/typescript-cheatsheets/react#types-or-interfaces

[julien]

Makes sense to avoid duplication 👍

Concerning those "interface" naming conventions, I asked because I'm curious but it's clear that it's a bit late for a change, and I know this has to do with personal preferences but I don't really like it. It's just a detail so let's ignore this 😂 In the future though, we might want to discuss this with the team (i.e. the entire team), it would be nice to use the same conventions for TypeScript everywhere we use it.

[matuzalemsteles]

Yes, I agree, I personally want to stop using it too, we can discuss this with the team later. Mainly, to adopt a standard for all projects.

[julien]

Great, seems like a good idea.

[julien]

@matuzalemsteles Can you also explain why we need this?

[matuzalemsteles]

This is related to this comment #4222 (comment), that's because again we support static and dynamic content, so the element's key can exist when it's rendered in dynamic content and doesn't exist in static content when it's not passed explicitly.

We can also create a unique key regardless of what level the component is at, if the component doesn't have a key or doesn't have an id. We also don't use the key generated by React, because it doesn't generate a unique key in-depth and we need the unique key to handle multiple and expandable selection for any scenario.

[julien]

This is related to this comment #4222 (comment), that's because again we support static and dynamic content, so the element's key can exist when it's rendered in dynamic content and doesn't exist in static content when it's not passed explicitly.

That's interesting. I think that regardless of the items being rendered dynamically or statically, we will need a way to identify them: for example to support reordering after a drag and drop operation, and probably more things. I'm going to debug this part in depth just to see if it makes sense for me.

[matuzalemsteles]

Yes, using the key we can even get that but for static content, I think we can skip adding reordering or DnD because it would be more complex to deal with, we would probably have to take children from TreeView and build a structure based on it and store in the state to allow us to manipulate and have an effect on rendering.

[julien]

I'll have another look at useTreeViewContext since I think it changed since last time, but I wanted to ask you what this selection.mount thing refers to, what's the idea behind it?

[matuzalemsteles]

I think we talked about it today but I'll go into more detail. I also put the reasons for this in my comment #4222 (comment).

The main idea behind this is to allow us to create a flat tree structure independent of the content being rendered, whether static or dynamic we will be creating a representation with a flat structure, the secret is that we also do not go through the items structure to assemble this because it would be costly and we would be waiting for it to finish to continue rendering, so think about it like a lazy and punctual creating the structure if you look at the implementation of mount it returns the function unmount if the component is removed from the structure this reflects updating the flat structure without having to traverse the tree again, so this is cheap.

Maybe I can change the name to try to make it clearer, I confess the name doesn't help but I'm terrible at naming 😅, maybe createLayoutLazy, createLayoutItemLazy...

[julien]

Yes, we did talk about this thanks.
The good thing is that this is cheap. I think we might need to move this to be able to use it from other places, because we're probably going to need that same mechanism for drag and drop and reordering. For the name we can think of something along the way.

[julien]

using the component's rendering stream to avoid traversing the tree in a separate stream

You lost me there 😂 What's a rendering stream? Got any useful links you could share?
The last sentence of this comment made more sense to me, I just think we should rephrase it a bit

[matuzalemsteles]

haha 😂, maybe the stream made things more complicated but this is similar to the concept of Streaming data, in other words we kind of have a stream of data that is sent from different places in the component structure to the function and we use it to map the layout.

Also what I mean by rendering stream is related to React rendering, for example, if we have a component structure like this:

<Node>
  <Node>
    <Node>
      ...
    </Node>
  </Node>
</Node>

We have a rendering stream, meaning React will traverse this tree to get the result of each component function call to assemble the VDOM and commit (that is when it actually makes the change in the DOM) to the DOM, instead of create this structure in a separate stream, for example, create before starting to render the components, rendering is blocked until we finish doing this operation, so we can say that we have two streams here, one that we use to create the flat structure and one that will render the components right? because in the end the rendering is the reflection of our data structure, for simplicity we can imagine this as we have two maps one to create the flat structure and one to render, both are nested maps because it is a tree which is already a bit expensive, so we use the rendering stream which I'm referring to rendering the components, ie I'm just using one nested maps instead of two.

[julien]

Thanks @matuzalemsteles that helps understand the concept. Now I just need to understand how to use it correctly but it makes sense

[julien]

Once again sorry for being honest, but I don't understand this (your comment)

[matuzalemsteles]

haha don't worry it's good to be honest it helps clarify the details.

To give this a better context, using some of the same comments above that complements it as well, is that since we're using React's rendering stream to lazyly create the flat tree structure, one of the things that happen is that we call this function on each item level in the ItemContextProvider in useEffect to get the invoking behavior only when the component is assembled and unmounted.

Weird behavior I didn't know is that when you have a nested render, you think useEffect will be called according to the order of declaration, for example from top to bottom, right? this is the opposite in React, only in hook calls, so the last element is called first (bottom to top), as in theory we would create the flat structure piece by piece, ie lazy, register the last element, we would not be able to register this element in the parent because it doesn't exist yet, so we pre-initialized the parent record of the current element, as we don't know what the parent of the current parent would be, we add undefined and when that element is invoked we'll update it with the corresponding parent.

[julien]

Yes, this is very related to the previous comment. I guess I need to debug this part a bit more just to see if it does what I think, and I also want to figure out where to access the whole thing (flat tree structure) to be able to use it for reordering (as I said in a previous comment, we might want this to be shared to be able to use it in other hook/places)

[julien]

@matuzalemsteles I also noticed in the demo that you linked that clicking on a checkbox will not only select it's children (makes sense) but also "toggle it". If that node was closed it kind of makes sense to open it and show all it's children selected, but if it's already open I personally don't think it makes sense to close it, "selecting" is one thing, "toggling between collapsed and expanded" is another one? Does that make sense?

[julien]

Didn't know about Omit but now that I do, am I understanding this correctly if I say that we omit the children property because it's already defined in ICollectionProps?

[matuzalemsteles]

Exactly, I couldn't extend from two interfaces that have different children declared, in which case I didn't want the children type of React.HTMLAttributes.

[julien]

👍 Thanks, that all makes sense.

[matuzalemsteles]

I also noticed in the demo that you linked that clicking on a checkbox will not only select it's children (makes sense) but also "toggle it". If that node was closed it kind of makes sense to open it and show all it's children selected, but if it's already open I personally don't think it makes sense to close it, "selecting" is one thing, "toggling between collapsed and expanded" is another one? Does that make sense?

This makes total sense, in fact, this is a bug that I haven't been able to solve yet, the spec says that if I click on the Node it will expand the element too, so I added the click listener to the element's parent, I also have a listener onChange in the checkbox, as in the checkbox the change also triggers the click event, I'm not able to stop the event so it doesn't reach the click listener, I've had similar problems in other places. I'll investigate it.

Actually, I wasn't thinking about marking the checkbox and expanding when it's closed either, it makes sense I think I'll keep this behavior.

[matuzalemsteles]

I just did a rebase to fix the conflicts

[matuzalemsteles]

@julien I send a few more fixes, it looks more stable now. I didn't keep the idea of when to select and have children expand the Node because I was thinking how this would work in case when we have a big deep tree and the "load more" when we don't have the data, we would have to keep making requests until the end and open, I think it is understood that you are selecting all the elements below and would also reduce this risk.

[julien]

This makes total sense, in fact, this is a bug that I haven't been able to solve yet, the spec says that if I click on the Node it will expand the element too, so I added the click listener to the element's parent, I also have a listener onChange in the checkbox, as in the checkbox the change also triggers the click event, I'm not able to stop the event so it doesn't reach the click listener, I've had similar problems in other places. I'll investigate it.

@matuzalemsteles no problem, I just wanted to mention it to have it on our watch list.

I just checked the update "stories" and it's fixed, if we need more adjustments we can see that in a future PR.

[julien]

@matuzalemsteles I fetched this again, to get see the latest changes, generally speaking this looks good to me, as I said for me right now the most important thing is understanding and organizing our "flat tree structure" so that it can be used easily (maybe it's already the case). I left all conversations "unresolved" although they are resolved, but I'd rather keep it like this for others to view the entire discussion until we merge.

I might add more comments with the things I discover.

[matuzalemsteles]

I fetched this again, to get see the latest changes, generally speaking this looks good to me, as I said for me right now the most important thing is understanding and organizing our "flat tree structure" so that it can be used easily (maybe it's already the case). I left all conversations "unresolved" although they are resolved, but I'd rather keep it like this for others to view the entire discussion until we merge.
I might add more comments with the things I discover.

Perfect no problem, I think I'll merge #4218 to allow us to continue with this and also merge this PR so we don't have to keep pulling PRs.

[matuzalemsteles]

I said for me right now the most important thing is understanding and organizing our "flat tree structure" so that it can be used easily (maybe it's already the case).

About that I added some comments about it in this comment #4207 (comment)

[julien]

@matuzalemsteles

Perfect no problem, I think I'll merge #4218 to allow us to continue with this and also merge this PR so we don't have to keep pulling PRs.

Please don't, we should close #4218 before continuing on this, some questions still haven't been answered.

I did check the (updated) changes and what bothers me in 19e2816 is that now we have "yet another way" to specify items.

Don't get me wrong, this "flat" data structure makes it easier working with the items (like moving them around), but I don't think developers are going to specify their items like this (and if we make them do it like that, they'll probably not like it).

[EDIT]

• Maybe that we could use this kind of structure internally to make it easier manipulating items, but I think we really need one only way to specify the items.

[matuzalemsteles]

@julien don't worry I'll just merge this if we have TreeView #4218 implementation in master 😉.

I did check the (updated) changes and what bothers me in 19e2816 is that now we have "yet another way" to specify items.
Don't get me wrong, this "flat" data structure makes it easier working with the items (like moving them around), but I don't think developers are going to specify their items like this (and if we make them do it like that, they'll probably not like it).

Yeah actually we only have two ways now 🙂. Well, I also agree that flat structure is easier but it doesn't seem to be a reality for some teams, but it seems to be a reality for other teams, Page Editor, Vocabulary, and Categories. For Page Editor it means that it's better for them, this is not a matter of obliging but it will be to reduce the performance bottleneck for Page Editor, so I think it's a good option to keep it, in the end, we're agnostic, so we don't depend on the data to implement the multi-select, or expandable features, maybe what remains is just the DnD but as I mentioned in the other thread about the DnD we may not be opinionated to move the element and let the developer implement, it seems safer.

[julien]

Yeah actually we only have two ways now slightly_smiling_face

That's exactly what I'd like to avoid at least in the first "iteration" and unless it's strictly necessary (and I don't think it is)

Concerning performance, until we test our component in the page editor, we have no idea if there's an issue.

[matuzalemsteles]

That's exactly what I'd like to avoid at least in the first "iteration" and unless it's strictly necessary (and I don't think it is)
Concerning performance, until we test our component in the page editor, we have no idea if there's an issue.

No problem either I don't think it's very necessary here anyway, I can remove it just until we have a better idea about it.

[matuzalemsteles]

I'm just reversing the change to add flat structure support and rename mount to createLayoutItemLazy. Once we have all the features, we can think about supporting the flat structure so as not to hinder the implementation of the next features.

[julien]

Hey @matuzalemsteles about createLayoutItemLazy, do you think are we going to have another function called createLayoutItem? Because if it's not the case, I'd just use createLayoutItem (I know naming is hard so we can take the time to choose a better name if you think that's not OK)

[matuzalemsteles]

about createLayoutItemLazy, do you think are we going to have another function called createLayoutItem? Because if it's not the case, I'd just use createLayoutItem (I know naming is hard so we can take the time to choose a better name if you think that's not OK)

No problem for me, I just added lazy to get the idea that this doesn't create the layout asynchronously but as it's being rendered. But maybe without it I can also convey this idea, maybe createPartialLayoutItem I don't know 😅

[julien]

@matuzalemsteles createPartialLayoutItem sounds OK for me, once #4218 is merged we can also merge this one, I've checked it several times (and #4255 is based on this, so I've had time to look at the code) and I don't see a problem here (well there's a conflict to fix, but that should be trivial).

[matuzalemsteles]

createPartialLayoutItem sounds OK for me

Ok, I'll change to this.

once #4218 is merged we can also merge this one, I've checked it several times (and #4255 is based on this, so I've had time to look at the code) and I don't see a problem here (well there's a conflict to fix, but that should be trivial).

Yeah, I'm going to rebase this one.

[julien]