feat(@clayui/core): Add initial sketch of components to TreeView #4218
Conversation
matuzalemsteles
force-pushed
the
issue-4117
branch
from
749fa3b to
629b4d4
Compare
| * SPDX-License-Identifier: BSD-3-Clause | ||
| */ | ||
|
|
||
| export interface IExpandable { |
There was a problem hiding this comment.
This file will be used to handle the tree data structure, to help control multiple selection and expansion of Nodes.
| <ClayModalProvider>{children}</ClayModalProvider> | ||
| <ClayModalProvider> | ||
| {theme ? <div className={theme}>{children}</div> : children} | ||
| </ClayModalProvider> |
There was a problem hiding this comment.
I'm still not sure about this change because we'll be adding a wrapper the application, but it's mandatory anyway for the application to actually be under isolation and scope from cadmin.
| interface ITreeViewProps<T> | ||
| extends React.HTMLAttributes<HTMLUListElement>, | ||
| IMultipleSelection, | ||
| IExpandable { | ||
| children: React.ReactNode | ChildrenFunction<T>; | ||
| displayType?: 'light' | 'dark'; | ||
| items?: Array<T>; | ||
| showExpanderOnHover?: boolean; | ||
| } | ||
|
|
||
| export function TreeView<T>( | ||
| props: ITreeViewProps<T> | ||
| ): JSX.Element & { | ||
| Item: typeof TreeViewItem; | ||
| Group: typeof TreeViewGroup; | ||
| ItemStack: typeof TreeViewItemStack; | ||
| }; | ||
|
|
||
| export function TreeView<T>({ | ||
| children, | ||
| className, | ||
| displayType = 'light', | ||
| items, | ||
| showExpanderOnHover = true, | ||
| ...otherProps | ||
| }: ITreeViewProps<T>) { |
There was a problem hiding this comment.
Unlike type on the other components in this one we are stopping using React.FunctionComponent as I had commenting in #4054. For more information on why, read also facebook/create-react-app#8177
|
TLDR; I don't see how using render props for this component benefits us. Glad to see this component starting to take shape! For the implementation of this component, I don't know if render props would be the best way forward. Personally, I find the render props API to cause more confusion than clarity for users. I may have missed something in your code or explanation, but I'm not really sold on using this sort of API. For your example above, what benefit does render props give us instead of using const ITEMS = [
{children: [{name: 'Blogs'}], name: 'Liferay Drive'},
{children: [{name: 'Blogs'}], name: 'Repositories'},
{children: [{name: 'PDF'}], name: 'Documents and Media'},
];
<TreeView
items={ITEMS}
>
{(item) => (
<TreeView.Item>
{item.name}
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>
// vs.
<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>If we move away from render props, we also no longer have to map and clone elements internally in our component.
I would offer a word of caution here about being too agnostic. I think it can be really beneficial to define a strict data structure so that we can control the implementation. Otherwise, we might run into several teams all using a different data structure and that will make it harder for us to maintain consistency across DXP. |
|
@bryceosterhaus yeah, in this initial sketch we still don't have all the features that the TreeView needs, as we develop the other features we will have more benefits from using Render Props, the benefit of this is that we reached a middle ground, for example: instead of building a Rigid API for a data structure for Tree and adding customization in other ways would lock the component for customization and hit new variants: const ITEMS = [
{children: [{name: 'Blogs'}], name: 'Liferay Drive'},
{children: [{name: 'Blogs'}], name: 'Repositories'},
{children: [{name: 'PDF'}], name: 'Documents and Media'},
];
<TreeView
items={ITEMS}
/>
// vs
<TreeView
items={ITEMS}
>
{(item) => (
<TreeView.Item>
{item.name}
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>You would have to write more code but this is minimal compared to the freedom of customization, as in the spec a Tree View can become a Vertical Navigation or add The render props is also not just to abstract the use of const [expandedKeys, setExpandedKeys] = useState(new Set());
<TreeView
expandedKeys={expandedKeys}
items={ITEMS}
onExpandedChange={setExpandedKeys}
>
{(item) => (
<TreeView.Item>
<TreeView.ItemStack>
<Icon symbol="folder" />
{item.name}
</TreeView.ItemStack>
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>This also allows for the middle ground to create the TreeView nested being agnostic of the data and allows visibility of the components being rendered to add any custom, for example: <TreeView
items={ITEMS}
nestedKey="children"
>
{(item) => (
<TreeView.Item>
<TreeView.ItemStack>
<Icon symbol="folder" />
{item.name}
</TreeView.ItemStack>
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>It might happen that some people want to customize <TreeView.Item>
{...}
</TreeView.Item>
// Output ->
<li className="treeview-item" role="none">
<div
aria-expanded="false"
className="treeview-link"
role="treeitem"
tabIndex={0}
>
<span
className="c-inner"
tabIndex={-2}
>
{...}
</span>
</div>
{...}
</li>
// With as={}
const ItemCustom = ({expanded, spacing, children}) => (...);
<TreeView.Item as={ItemCustom}>
{...}
</TreeView.Item>
// Output ->
// Just pseudocode, disregarding the markup result, just an idea of the benefit of it.
<li className="treeview-item" role="none">
{...}
</li>In the end, we want to have data visibility but allow customizing the components without having to create a lot of APIs as we did with DropDown to customize parts of the internal components, this is related to the collection idea that we'll be researching soon #4112, I think the comparison here would be between: // CustomRenderers would probably be even more complex
<TreeView
items={ITEMS}
itemAs={ItemCustom}
customRenderers={{
item: (item) => <strong>{item.name}</strong>
}}
/>
// vs
<TreeView
items={ITEMS}
nestedKey="children"
>
{(item) => (
<TreeView.Item>
<TreeView.ItemStack>
<Icon symbol="folder" />
{item.name}
</TreeView.ItemStack>
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item as={ItemCustom}><strong>{item.name}</strong></TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>In other words, the whole idea here is to reach a middle ground between the benefits of low-level and high-level, having On looking confused I'm not sure if that would be confusing, it's quite normal in the React community to use Render props and it's an old technique so I hope people who develop on React don't find it weird.
It's a valid point, but creating a rigid API at the component level is very risky because we have many backends already ready, for example, Categories, TreeView for listing sites and other things in the Portal, both have their particularity, mainly visual, creating an API Rigidity would make adoption and use much more difficult, as happens sometimes for |
|
So it sounds like your main point in wanting to go with render props is for customization and flexibility without having to add numerous props. I think that is a fair point, however, I am not entirely sure that render props actually solves that problem and I think we will run into the same issues internally of having to make "magic" work.
Do you think we could also achieve these features by using hooks? You mentioned that one feature that would benefit from render props is "facilitate Expandable and Multi-Selection". I think we can also achieve this via context though.
I'm not think of adding a strict <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>From my understanding, render props are useful when you want to allow users to take control of how the markup renders. For example, we already do this in MultiSelect with I am also trying understand what the key benefit is in using render props over our existing patterns. IMO, the consequences and difficulties I see of using render props:
Additionally, one thing that might help drive the API here is picking the 3-4 major use cases in DXP and then creating pseudocode implementation of how you would want those to look. I fear that if we immediately opt for the "freedom of customization", it will quickly grow into several different implementations and causing more of a headache for us. Anyways, I am curious to hear some other thoughts on this as well. And I think it would be helpful to hear from people who maybe don't use react as frequently as we do. Sometimes I find having too much experience with a certain technology actually makes it harder for me to see the wide range of viewpoints 😂 . cc @diegonvs @kresimir-coko you guys have worked with clay and dxp quite a bit, what do you think? |
Hmm I'm not sure if we're going to face the same problem, for example, in theory our components shouldn't have APIs like Unlike for rendering props, we don't have this because the components need to be explicitly declared with a minimal design, so the developer exposes the APIs of the components that are used.
We can't build via Hooks, because most of the implementation is based on As I said this will help work with the other features, I need the TreeView to have visibility and take some control of the rendered elements to allow us to add features like virtualization that we'll need to work with, also nested rendering. As I've mentioned rendering props strikes a term medium between high-level and low-level, that's probably where we're headed. We want to take control by providing dynamic features like high-level, static like low-level, and compositing like low-level that removes the need for props like Tomorrow I will be sending the feature to add the expandable TreeView and multi-selection, so things can be clearer.
Hmm I'm not sure about that, because essentially Clay doesn't cover all use cases, it's also not 100% true that everything will follow Clay's markup, I've found myself several times adding extra customizations, new features pop up all the time, having a restricted API prevented a lot of use for new cases, for example, DXP Cloud has been using Clay to build their Admin and I often saw them trying to extend cases or needed to customize some parts, it's a particular case but this also happens with frequency in the DXP. I don't really like the Of course, rendering props brings very high flexibility that can hit a lot of use cases but I think that's what we want to go forward with Clay, as Dialect comes along and Lexicon expands its spec to something more abstract and Clay is used a lot. Naturally, developers want to use Clay as a composite to allow markup change, over the 2 years of v3, we've added several APIs in our components to pass props to sub-components, for example, a problem we saw when a component aggregates many behaviors is that we didn't know where else to add
Yes, this is an initial test that I want to work on to reach a middle ground, we need to create a middle ground as Clay moves towards adoption in more places and this is an important factor, for example see the idea of #4112 collections , we want to standardize many of our components, see also my proposal on Clay evolution that I shared when you was on license https://paper.dropbox.com/doc/Clay-strategy-and-solid-foundation--BQlimpbyTIEcxdMBeyKF2xbAAg-UbYmJiDGPSVzuxZVPUgYl 🙂 We only have one use case that was in <ClayDataProvider link="https://rickandmortyapi.com/api/character">
{({ data, error, loading, refetch }) => {}}
</ClayDataProvider>The idea of collections will probably hit most of our components to lessen the cognitive load of using Clay and having to learn how to do things in different ways in each component, this is a little bit inspired by the React Spectrum collection system.
Well I wouldn't use DXP as a place of good standards 🤪
I'm not sure about this, we have documentation on Rendering props in many places, mainly in React's own documentation as an incentive, Formik, React Router is supported, React Spectrum abuses it a lot too, downshift, react-beautiful-dnd, react urql, Kent C. Dodds has many articles on this also, React Apollo has supported for a long time and other libs as well, in that I think it's a good pattern that is already used a lot, of course, hooks has eliminated many use cases of render props.
Yeah, that's a good catch to understand, normally I look outside also to understand what others are doing, DXP is still very new in React so it's good to bring the use cases from outside to here, I see a lot of wrong patterns in DXP, so I don't bring much of what's there to here or serve as a decision-making process, I don't try to bias myself with the standards either, but then I do the polishing.
Yeah, that's makes sense. |
|
I'm reluctant about using render props because:
|
|
To make it clear again 😅, using render props based on For example the variations listed in the specification have cases that the current markup will not support to render a node, because we apply height rules and being variations Clay CSS doesn't necessarily need to support this but at the component level we owe at least the freedom that the developer can change to your own markup, I think we're okay with that but the point is, the tradeoff of not going with render props for this case means we're going to have to figure out parts of the internal components that the developer can tap to change, create
Yes, this is true for libraries that deal with data but it's not true for cases like this that deal more with markup, if I could handle all this via hooks, I would definitely do it. We did the same for the As I've commented, other libraries still support it because hooks are not used in class and others use it as a recommendation because it depends on the markup to handle the strategies. Using this here will allow us to make a lot of improvements, for example, we can cache the rendering done of the component and avoid calling it again, React at this level doesn't take care of that.
Yes, this is a valid point, but there is a point that we should consider that techniques like these people should know and we shouldn't ignore them to introducing, maybe if I had added HOCs I would have agreed the degree of complexity that this adds is very high, the Render props is more subtle so I'm still insisting on it and we can document it well, I don't think that would stop people from using it. Just to make the use of render props clear, it also applies to: <TreeView render={() => <Item />} />
<TreeView customRenderer={() => <Item />} />Technically any prop with a function the component uses to know how to render is a render props, so it won't be hard for people to use.
I'm not sure I understand. Rendering props is very subtle here, so I think it doesn't increase complexity I think it has more gain than increasing complexity or messing up a customization for example.
In fact, I'm the opposite 😅, using "load more" has an advantage of loading a minimal payload so its start is less memory consumption, as the user interacts and loads more data is where the problem lies, the React has problems rendering large lists above 200 or 1000 lines, so it starts getting slow for interactions and for scrolling, for example at every new rendering React will invoke the functions/components again to create your vdom, there are cases where it optimizes and doesn't call the entire tree but still, there are many renderings, in the meantime, we will be making a I'm not against "load more" but just implementing it won't help, the last time I analyzed the performance issue the client had a list of 2000 or 1000 categories on the same level. I think we can implement both. |
|
Overall, I trust your decision in what you think is best for this component and I am happy to be wrong about my concerns. MAIN THOUGHT HERE BELOWI will reiterate an idea I had above though. I do think it would be helpful to test out this API first by taking the top uses in DXP of TreeView and create pseudocode of how we would implement them. I would even encourage creating both the render props version and non-render props version. By having these examples, we get a clear picture of what the end user is working with and think we could make better decisions based on that. It would even be helpful to present those pseudocode examples to some teams and ask them what they think of the two different APIs. spacing for dramatic effect 😮 Definitely not as important as my point above, but I wanted to note on a few of your comments...
I'm not too convinced on this. I do think hooks can cover the majority of cases for render props, except for customizing markup. Even Kent seems to agree with this idea as well. The example he uses is almost the same use case as
Just to note, whether render props are done via the children prop or through a different named prop, the API is the exact same. So there is no different between using render props However, I do think there is a sort of "implicit" difference between using the props. When using these components I have a mental model of
If this is true, I think this should worry us. If we are expecting people to use Clay components and allow for non-clay markup, support will be really difficult.
Remember that DXP is Clay's number one user and the primary driving factor for Clay. Especially more so now than when v3 was started because both AC and Commerce have been merged to master. So even if DXP doesn't have the best standards for react and JS right now, we need to make sure we are being overly strict in our APIs and helping DXP get to a place of good standards.
I think this speaks a bit to feedback cycle and something that has been historically difficult at Liferay. We need teams to cycle back to our team to provide feedback so that we can provide real solutions for their needs. Often what leads to back practices is teams run into issues with the available API and instead of letting us know of their use case, they hand roll their own solution or manipulate Clay in an undesired way. /end of comments 😂 |
|
@bryceosterhaus just to make it clear that I'm not saying you're wrong 😅, I think we're just trying to figure out how best to do it. Anyway I'd like to try to go with that ahead, this base will need to apply to the other features like #4222, you can preview the storybook to see more use cases. Merging this doesn't mean that the API will be like this either, we can change it at any time, if we really verify that rendering props is really bad compared to a customRenderer or just a high-level component to cover everything, I don't know, that component will only be available too as soon as we merge all the features then it's safe.
Yeah, yes I think this is valid and we can try, I think in the end both will be able to fulfill what already exists in DXP, I'm more concerned with the Lexicon specs, the TreeView that exist in DXP are the old ones and with the old visuals, some still in AUI, so we're going to have to update them anyway, so it's hard to know if it's better or not. I'm also concerned not only with the people who use this one now but also with the new people who will use it, considering the spec, the Tree View covers a lot of use cases, as well as replacing Vertical Navigation. So it's a new thing.
Yeah, in theory, we could but that would require extra work, I think it would be "uglier", and a really weird API to handle renderings, for example. const {items} = useTree({
itemRenderer: (item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)
});
// I can't imagine what this part would be like to render
return (
<TreeView>
{items.render()}
</TreeView>
);
// or
return (
<TreeView
items={items}
/>
);Well, anyway passing a component to a hook seems counterintuitive since the hook should only deal with logic, anyway, the render props still has its use cases, as Kent also claims...
Yes I think that might be true, but we should also take into account that rendering props by children is more intuitive it's implicit that you can compose between different levels, it's the same thing we achieve with low-level components, implicitly you know you can compose the components, replace, the value is exactly there. When we have render props based on {(item) => (
<TreeView.Item>
<TreeView.ItemStack>
<Icon symbol="folder" />
{item.name}
</TreeView.ItemStack>
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
// vs
<TreeView
itemRenderer={(item) => {
if (item.type === 'folder') {
// There is a problem here, that maybe in this case, we can't allow
// the developer to explicitly pass the .Item, because in the markup
// we need the Group to be a child of the Item.
return (
<TreeView.Item>
<Icon symbol="folder" />
{item.name}
</TreeView.Item>
)
}
return (
<TreeView.Item>{item.name}</TreeView.Item>
);
}}
groupRenderer={() => <My.Group />}
/>
Yes this is a big challenge to achieve, one of the things I'm thinking about is providing more solid components to Clay's foundation as a way to allow teams to still create products with new shapes and different variations, I don't think we have how to prevent this... but we can decrease the amount of extra customizing. I even listed some based on what other libraries have been doing for a while.
Yes of course I'm not saying it has bad Clay standards but I mean from its own components, Clay can't achieve that because the components are built exclusively for the product, what I mean is that we can't be inspired by that because we get biased, as I said the React base in DXP is very new and in order to help teams apply good patterns we need to bring them and add them in DXP. This is different than building on what was built into DXP, does it make sense to you? just to be clear again, I'm not ignoring DXP as our clients are actually my biggest concern around all of this.
Yes, I totally agree, this is very difficult to deal with. I hope that with our documentation improvements, APIs and trying to show more examples and explain more why things encourage good practices. |
Yeah I'm not advocating for using hooks to accept components or handle renderings, I think hooks(specifically context) could be used for tracking expanded/selected items or other logic like that.
In my experience I wouldn't say this is always the case with render props and would defer to users of the library to decide how intuitive it is. I've used different libraries that used render props heavily and struggled with using the library. But regardless API choice, it is our responsibility in making it as intuitive as possible for consuming developers. Both with render props and not.
Oh don't worry, I would be the first person to say we have some bad standards of both React and Clay in DXP 😂 . We just got to keep on working slow and steady to get it in a good place.
Yep! Totally agree with you on this. I was just bringing up DXP again to make sure we are constantly thinking through that lens and how its requirements will shape clay. |
Yes, that's what I'm doing in #4222.
I would love if we had a more close collaboration with other teams, like releasing beta versions to test the authenticity of the component API before marking it as stable, it might be a practice to do with this component, we test on some more critical components and check with developers for component ergonomics and stress tests for cases, probably stress tests we can do outside of DXP without any problems. |
Hey guys, I know it's not urgent and a bit out of scope with this pull request but I'm definitely interested in seeing a list of those bad standards to avoid adding more. |
|
@julien thats actually probably a good idea to document some real world examples in DXP of bad practices. I'll add it to my todo list and hopefully can spend some time on it in the next few weeks. |
@bryceosterhaus @julien I think it's also a good idea, it can serve as an insight for us to improve the direction of our documentation and maybe also to understand some points, we can create an issue to be able to add the cases we found and we'll discuss it there. What do you think? |
|
I can't help much in the discussion of react renderprops. However, if we want to test our hypothesis in real DXP scenarios where we have trees with potentially tons of items and flat hierarchies, I'd strongly suggest to consider page tree in product menu and the asset category selector. Those places have well known performance issues, repeatedly reported by our customers Note that the former case is not using our react Treeview but the much older As a side note, do we plan to enhance or substitute the current react Treeview? I'm asking because that component is used in other places. |
Thanks @dsanz, I'm considering them, yesterday I was looking more at your analysis to see the use cases in DXP, in fact the AUI translation is very difficult to do. But I'm also considering the current TreeView implementation in DXP, it looks like it's used for some specific places and the implementation doesn't deal with performance either actually has a lot of problems due to the way it was done so my idea is to replace them with our implementation that it will be more efficient for long lists. Even yesterday I was recreating some of the use cases with the current implementation of this PR and the expandable node, I will send more example of this in PR #4222 today or tomorrow. |
|
Hey @matuzalemsteles, took me some time to reply, but I'm also adding a comment concerning a render props based API. As @bryceosterhaus and @nhpatt already pointed out, I'm also not sure our users are familiar with the concept, and I think we should take some time to analyze other alternatives before making this public - @bryceosterhaus 's first example makes me think we could make this simpler and the fact he commented more than once, convinces me that this is not something we should "take lightly". On a related note, the only real life example I have with these "less common known" React patterns is when we switched to HOC's in alloyeditor, and even though he we're only going to be using render props in a single component (vs in a complete project), I wanted to mention it because I think it's relatable. Analysing and implementing current uses cases in DXP sounds like a good way to have a better idea of how this component will be used and how we could make it as "easy" as possible for our users. Along the way, we'll be able to answer the question we seem to all have in mind: Is there anything we can't achieve if we move away from render props? Concerning my first comment about "good/bad practises", I think this could be done as a collaborative "cross squad" initiative, since we've got example in various places, as I said it's not something urgent and we'll need time to gather our thoughts, but it's going to both help us and our users. Let me know what you think when you can 😉 |
|
Hey @julien I understand your point of view and are valid, I just disagree that these patterns are less known, maybe because in Liferay we started to directly use hooks compared to components with classes where these patterns were used more, mainly HOCs that were practically null after hooks, actually introducing hooks was mainly to lessen or remove the complexity added by HOCs but they're good alternatives when we're talking about components with classes, AC mostly used a lot of this pattern it still has a tight coupling with it. They still use a lot, can't get rid of everything 😅. In regards to rendering props I'm a little surprised that we're finding it more complex, adding a function should be subtle with this technique much closer to composing components and still abstracting the extra complexity that would be added, the first example of @bryceosterhaus is the low-level composition that we have the same for the other components, actually you can do that also with this implementation but the problem with this first example is that it won't cover everything we want, for example Tree nesting, virtualization, render cache, as we wouldn't have control of the data we probably couldn't also implement "load more", maybe some of those implementations could be possible without rendering props but we would probably be trying to circumvent or create APIs to know the data. For example: Considering that we don't have Render Props I see two strategies that we can follow and I'll try to score again why I'm not following them:
The strategy of rendering props for us in this component is to allow us to reach the middle ground, creating a certain balance between the advantages of this strategy. So the benefit here is that rendering props allows for composition compared to high-level. The point about it's "nice to look at the use cases in DXP" I agree, I've looked at some, see an example I created in PR and my comments on #4222, although looking at DXP is that a part is written in AUI, another one is using the TreeView implemented in React, the difference is that the TreeView in React of DXP is not performative, it follows the idea of high-level anymore and does not implement the Lexicon specification, Lexicon makes this component very flexible to reach new variants , so just looking at DXP won't help us know what the best API is let's just find out that for the DXP case they both work, it's worth remembering that the Lexicon spec has use cases that haven't been implemented in DXP yet, for the AUI components we're going to replace with this component as well as the new look, behaviors, so there's no way to learn much from them. I still disagree that "render props" or "HOCs" are unknown, that's because both are in the official React documentation, we have thousands of articles talking about them, the vast majority of courses we can find on advanced or beginner React can introduce render props, look at egghead's courses for example or even our "React Fundamentals curriculum" documentation has a React training course that talks about rendering props and even though we may have some people new to React, we shouldn't take it as an argument not to introduce a known pattern that can help us, if you don't know a new strategy or new patterns I believe we should introduce and teach and document. For example, React Spectrum brings render props to its Collections implementation, we have new strategies for fake components to allow you to reuse the same component in different contexts, see this thread https://twitter.com/markdalgleish/status/1370951759284162565. I'm not saying this should be the choice my point is that this is valid and probably not complex for how much it delivers compared to others and for that case it will allow us to reach a middle ground. Anyway I understand you are worried about what would be simpler, but look at the pros and cons of each implementation mentioned, the TreeView is complex so just let the developer handle the rendering itself via |
|
About virtualization (because I'm worried about deciding an implementation based on supporting a specific feature), after some conversations with Dani of current uses he does not think (me neither) that opening the full tree is a real use case. The real use cases/performance issues (Dani has more input on this) are related to drill down in the tree, exploring from the root to a leave or siblings... that's why he suggested start with "load more" as the best performance improvement to start implementing it, because it seems will offer the best ratio of effort/results. |
|
@matuzalemsteles thanks for the detailed explanation it helped clarified some of my doubts: I know that you've used React and know all these patterns, but I still don't think that the case for everyone, so to conclude, if render props allow us to do what we need and as long as it is documented / easy to understand, then it seems like a safe option. I also understand that this is a "v1" implementation, meaning that things can still be changed along the way if we find something better, and related to @nhpatt 's latest comment, maybe virtualization is something we don't want to think about unless it's absolutely necessary. |
|
@nhpatt hmm no problem we can start with "load more" and test, just to make it clear "load more" will only load a sub-tree or is also related to the list, for example don't load more than 20 items of the same level, and if you want to load more, press "load more"? My concern is more when the user opens a lot of subtrees this also causes the rendering problem, it will have the same effect but we can see how the "load more" will go out and do load tests. Implementing "load more" also changes our search behavior right? doing a runtime search would not be ideal because we won't have visibility of all the data I think the same applies to what the paging scheme for the tree will be like, maybe we will have to implement more things on the DXP side to handle this. @julien yeah, if we really go with render props we will document this well, it will be much more detailed than we do with our other documents. Anyway we can leave the virtualization until we understand if the "load more" will be enough. |
That's what I think it is not an use case
This should not be the problem if we use the existing APIs |
Oh great, is there somewhere I can look or test? that would help how I'm going to design this. |
|
All new APIs are hierarchical in nature: https://app.swaggerhub.com/apis/liferayinc/headless-admin-taxonomy/v1.0#/TaxonomyCategory/getTaxonomyCategoryTaxonomyCategoriesPage They allow to get the first level: https://app.swaggerhub.com/apis/liferayinc/headless-admin-taxonomy/v1.0#/TaxonomyCategory/getTaxonomyVocabularyTaxonomyCategoriesPage, or everything with the flatten query parameter, and then any level of the hierarchy (https://app.swaggerhub.com/apis/liferayinc/headless-admin-taxonomy/v1.0#/TaxonomyCategory/getTaxonomyCategoryTaxonomyCategoriesPage) or every level from there with flatten too. There is even talks for allowing n-levels (whatever you request) from any level of the hierarchy. Should be easy to add to all the APIs in one sweep, as everything is indexed in Elasticsearch. |
|
Oh interesting, thanks a lot @nhpatt! I will look at this. |
matuzalemsteles
force-pushed
the
issue-4117
branch
from
66f4585 to
a803aad
Compare
|
Hey guys, I know we certainly haven't come to a consensus on whether we should go with rendering props using children or any other solution that solves the use cases of this implementation. As a point, I still believe that using render props for this implementation will be very valuable, as we are already working on the TreeView implementation I wanted to merge this also which reinforces that merging does not mean it will be the end result, this is an initial sketch of implementation it may be that at the end of the result we can change everything or just keep what exists. So the idea of merging is to allow us to keep going with the TreeView implementation, I've already implemented a lot in PR #4222 that reinforces some of my views that I've commented on throughout this discussion, it also allows us to still continue testing. Please leave your thoughts. |
@matuzalemsteles what is the status on mocking up some pseudocode like we talked about above? IMO, its hard to evaluate the implementation of TreeView and the API to go with if we don't have pseudocode of the use cases to go off of. So I really think it is in the best interest for us to mock up both render-prop and non-render-prop pseudocode for our DXP use cases. |
As I mentioned, I made some use cases of the Page Editor that has the TreeView to list the Page Elements but this is still not 100% faithful because I didn't do all the visual part which is very different from the TreeView, it's probably product specific so we should be very flexible here. See https://github.com/liferay/clay/pull/4222/files#diff-e61ee3872daec9ec10795e2842035b8a7242083798a38a2ba1a3e22a751a8b34. A lot went with the implementation of expandable and multi selection, see PR #4222. Also as I commented the DXP use cases will be replaced by the new implementation, new look, new behaviors, so there is no way to faithfully compare or use as a basis. As I said, anyway we'll be able to cover what's in DXP because the use cases are simpler compared to the new specification. My biggest concern is to cover what's in the new spec and the new variants. |
matuzalemsteles
force-pushed
the
issue-4117
branch
from
a803aad to
1994873
Compare
Yeah I saw the other PRs, my concern is that we are going too quickly after implementing a TreeView component without determining how the end user should use this component. In order to best determine the API and its flexibility/strictness, it is going to be best to mock out the pseudocode of the top three use cases in DXP. If mocking out that pseudocode for DXP's top use cases is too difficult to do, I think we are especially getting ahead of ourselves and won't benefit the product teams if we are unable to steer them in how to use the component. |
I also agree with @bryceosterhaus, from what I saw most teams are using the I have a feeling that by making this new TreeView component too flexible (by accepting both dynamic and static content) we're making it also too hard to implement. And if the "basis" of this component isn't correct it's going to be very hard implementing more features. |
|
It is important to note that we are not only creating this component to cover what already exists in DXP, many of them will also use the new pattern, this will cover other new use cases that were specified in the documentation, the static content is to allow cases like Vertical Navigation or just a TreeView that doesn't need dynamic content https://docs.google.com/document/d/1fggZBRGAIkGPbnkRrGEUOT0hHpkF-YjFmREee0XCoI0/edit. As I've commented on in the PR description and throughout the thread, static and dynamic content is the middle ground between high-level and low-level components, so the basis of this component is inspired by what we've already had and tested over the 2 years from Clay v3, also the way this API was designed was not only created from my mind but also inspired by what the Adobe team has been doing with React Spectrum https://react-spectrum.adobe.com/react-stately/collections.html. Regarding pseudocode code for DXP I come back to talk again, the TreeView implementation in DXP will be replaced by the new implementation, so it will be a new look, new behaviors, using as a basis will not help to design which API should be the most correct, but I agree it can bring insights how to improve certain behaviors and as I said I've already looked at these cases in DXP to design this component but it's also not enough to say what the API should be like because the TreeView specification designed by Lexicon adds new behaviors and cover different use cases, so considering just the React component of the DXP TreeView, we have the use cases:
This is the list of all occurrences of TreeView usage in DXP, the vast majority use
These listed components are not so critical when compared to what they are using AUI, so it is very difficult to compare with any implementation, as we will also change the look in some cases. Consider some examples above using this API: PageStructureSidebarThis implementation uses DnD, they have their own implementation apparently, it's not very clear. They also assemble their tree structure at runtime which is a very big performance bottleneck because they only have their flat structure (perhaps a point to look at how to support rendering using flat structure). const nodes = [
{
// This is also quite costly for them, this would be like multiple
// selection but being able to only choose one item, we can implement
// `selectionMode` with `multiple` and `single`. But they make this
// selection using their own data and state.
activable: true,
// More data similar to this structure
children: [...],
draggable: true,
// We don't need it
expanded: true,
icon: "container",
id: "98a7c550-4605-2f6a-5c71-c91bac9ea19e",
itemType: "layoutDataItem",
name: "Container",
parentItemId: "fd5768d1-a84c-1f55-ee94-25a0858a0590",
removable: true,
type: "container"
}
];
// From Page Editor for interaction
const hoverItem = useHoverItem();
const [selectedKeys, setSelectionChange] = useState<Set<React.Key>>(
new Set()
);
const [expandedKeys, setExpandedKeys] = useState<Set<React.Key>>(
new Set()
);
const Group = ({className, children, ...otherProps}) => (
<ul {...otherProps} className={classNames('lfr-treeview-node-list', className)}>
{children}
</ul>
);
<TreeView
expandedKeys={expandedKeys}
expanderIcons={{
close: <Icon symbol="hr" />,
open: <Icon symbol="plus" />,
}}
items={nodes}
nestedKey="children"
onExpandedChange={(keys) => setExpandedKeys(keys)}
onSelectionChange={(keys) => setSelectionChange(keys)}
selectedKeys={selectedKeys}
selectionMode="unique"
>
{(item) => (
<TreeView.Item
onMouseLeave={() => {
if (!isDraggingSource && isHovered) {
event.stopPropagation();
hoverItem(null);
}
}}
onMouseOver={(event) => {
if (!isDraggingSource) {
event.stopPropagation();
hoverItem(item.id);
}
}}
>
<TreeView.ItemStack>
<Icon symbol={icon.icon} />
{item.name}
{item.removable && (
<TreeView.Action>
<RemoveButton node={item}/>
</TreeView.Action>
)}
</TreeView.ItemStack>
<TreeView.Group as={Group} items={item.children}>
{(item) => (
<TreeView.Item>
<Icon symbol={icon.icon} />
{item.name}
{item.removable && (
<TreeView.Action>
<RemoveButton node={item}/>
</TreeView.Action>
)}
</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>I haven't added everything they have because it's too big in terms of logic that isn't necessarily tied to markup, one important thing to note is that DXP's TreeView component follows the following API: <Treeview
NodeComponent={StructureTreeNode}
nodes={nodes}
selectedNodeIds={[activeItemId]}
/>For the case of the Page Editor, they overwrite all the CSS to get the look they want, so from the TreeView there is little markup and little CSS apparently. The point I want to make is, using render props allows you to touch more components compared to a high-level API and get the same high-level features in a low-level style composition, that's important because if that were the case of trying to replicate the TreeView specification and Patrick's markup using a similar API we wouldn't be able to because we need to change the Group and Node markup, the advantage here we have is that we allow more use cases, lets avoid a kind of weird case in the PageStructureSidebar implementation, it adds an For this specific case, I believe that in visual terms it is much easier to get what they need and for sure it would simplify a lot of implementation that they have added. Components with
|
|
@matuzalemsteles that's a lot of information 😄
I had a look at it and it does make sense, but it looks like this is "incomplete" (on our side), we're still missing the ability to insert, remove, move or update items, as described here, and I think that's going to be needed for drag and drop (that's just one example use case, we might have more). Another thing I keep on seeing is item with ids (or parent ids) in either your examples or the ones from the link you posted: I'm going to have a look at the last changes you made in #4222 to see if we have that implemented (using keys didn't really convince me, more on that below).
About that, I agree with you that if Lexicon defines new behaviors then there's nothing to compare with, but I also think that the easier the transition is (from current TreeView to new TreeView) the better it will be for everyone, because otherwise we'll have to do these replacements or at the very least assist the team during the migration, and I think that @bryceosterhaus was pointing out that bit (correct me if I'm wrong).
I don't really understand what you mean with "critical", because for me each use case has the same priority, and I think that if we get this component's API right then it will be easier to migrate (I also have no idea about when some AUI modules will be migrated to React but I think that by that time, we'll already have seen a few usages of the new TreeView in React modules). When looking at the examples for the existing use case you listed, it looks we're going in the right direction, the only thing that I would change is that the "drag and drop" would be handled internally, of course we can add callbacks to let the use know when something happens (in the case of page editor, if you remember, the drag and drop functionality is there to add "items/widgets" to a page - something we actually worked on together a while back 😄 I'm not too worried about that part), as I already said, I'll have to look at your last changes because I keep on seeing ids everywhere in these examples, but I didn't find that in the component (I know we use Apart from that I think that trying to implement "selection/multiple selection" and "drag and drop" before deciding on this "initial sketch" is too confusing (it's actually not possible) because we now have three conversations in different places concerning different things, and it's probably better to finish one task before opening another. I'll do another round of "reviewing" with this branch fetched locally and come back with more feedback. |
@julien yeah, still very incomplete, they deal with a lot, I thought I'd start creating here their implementation is much more complex than that and more generic that allows reuse in more places, I didn't want to do this now because it demands a lot of work and also we would have to analyze how this would fit into our library, deal with breaking changes and also see if it would be a valid option but I think it has great benefits. This is related to the #4112 collection research to create consistency between components, it is related to the milestone of API Consistency as well. But let's take it easy with this because it would be a big step and we can study more slowly. This would also be an RFC as an initial proposal after we collect the data and see the initial state of our components that are related to the collection.
Yeah, that's true and I agree with you, I think the most complex case we have is the Page Editor which has a lot of implementation around it, both data manipulation and to implement their DnD, so maybe we can reduce this complexity with some resources that we have, if I need it I think I'll work on changing their TreeView to use ours and show how this can simplify and take out some defaults, also after I've added flat structure support it can help them a lot. But the rest of the other components are simpler the other more critical ones are in AUI so it's really hard to compare, it would have to be rewritten anyway.
Yeah, actually the critical rating here is more performance related, for example see the review of @dsanz https://liferay.atlassian.net/wiki/spaces/ENGFRONTENDINFRA/pages/1586562122/LPS-133328+Scalability+improvements+in+the+TreeView+component about this. But yes, in terms of usage they are all the same. And actually, the Page editor is not classified as critical but it is also major anyway but only two critics, one in AUI and one in React. But overall the Page Editor is the most complex.
Yeah I still remember 😅, I think having DnD can decrease their complexity too, maybe we have to expose a callback really like you said.
Yeah, just commenting why we talked about it today 🙂.
Yeah, I think this may be causing some confusion but I believe that even if it changes the way the component is presented or the way to use it, internally we will still continue with a good part of the implementation if you look at the components we have are like low-levels the difference is that in the presentation we allow them to have more resources, so changing that won't impact a change in expadable or multiple selection for example. Why this is more related to design in the component's public aria. |
|
Lots of info, and I will try to only address a few things in order to avoid confusion. Just to note, I am primarily thinking about this from a high-level and not necessarily at the implementation level, so we may be miscommunicating a bit on that and I may be overlooking things at the implementation level. I am primarily thinking of these two questions here:
Oh sorry, when I say "the top three cases in DXP" I don't necessarily mean what is in DXP right at this moment but what are the top three cases that we need to provide for DXP.
Oh no, not at all. I think render props and this API will make it possible to accomplish the desired behaviors and allows for a great amount of flexibility. My concern is making it too flexible and open. From my experience, the more we let developers have flexibility, the harder it is to maintain (this is why we added npm-scripts). If we give a really flexible API to 3 teams, they will implement the component in 3 different ways, which makes it harder for us to maintain long term. In an ideal situation, I think an open and flexible API is great, but it has a draw back because it makes it difficult to enforce consistency across products and teams. Remember that "Any use case" can be a double edge sword. It can be great for flexibility, but we need to also make sure we don't let teams become too inconsistent from one another.
👍 👍 In the end, I trust you know what is going to work best here for Clay and so I am happy to go with whatever API you think is going to be best. My goal here is thinking in context of DXP and making sure we are pushing teams towards uniformity and consistency. The more teams deviate from one another, the more chaotic it becomes. |
|
Hey @matuzalemsteles, @bryceosterhaus is right, this thread is really getting long. What Bryce wrote in his last comment is what I had on my mind, I think these two sentence resume it well
Yes and I agree that having an API that is very "permissive" is not going to help. We should try to answer these questions (before starting on other tasks)
|
|
Ok, I will try to clear up some of your questions, I will try not to be long but objective 🙂!
This is quite complicated to answer objectively, historically we have delivered two levels of layers but we can count with 3, HTML, low-level and high-level components, each one has its particularity, we know that the choice of both has tradeoffs, the high-level components seem to be used less compared to using low-level, as I said it seems, this is a bit of my perception when I was in DXP, for example: consider DropDown in DXP:
Low-level components have more occurrences, but there are some interesting insights here: high-level components we agree are simpler right? because you write less in theory we should see more use cases for it, the cases in DXP, you see they are very simple, some examples I saw people using and doing a remapping of the data to move to the high-level component I saw In several cases like this, this draws attention because being governed by the data in the component shouldn't be the responsibility of the component but of the API and wanting to become uniform among the teams is also a responsibility of consistency of the backend. Another insight into this is that the low-level components they're actually more permissive than rendering props, so I don't consider rendering props "guilty". So the low-level of DropDown has many cases in DXP, what we can see from the examples I found is that the vast majority, maybe 90%, uses DropDown as a dynamic content in different ways, in various patterns, why? well, What I mean by this insight is that if it's helpful and easy API, well apparently people like composition, and using low-level components is easier because they can adapt to their designer's standards, high-level can be faster but developers apparently don't use it just because they're fast. This answer was not very objective but this is a difficult question to answer so I needed to bring our historical context and I say again I don't see rendering props being difficult to use, it is just a piece to allow us to find ourselves in the middle of these two problems and cases that we see. If it's easy, yes, it's useful, too.
I would answer this separately but it's very much linked to the previous comment, but in addition to this question, it's hard to think what would be consistent for you! For example, visual consistency? consistency of using APIs, what would not be consistent? TreeView is very flexible at the macro level in terms of specification, so it can behave like a Vertical Navigation component, for example, visually it can have different uses, so we need to be permissive. If you take into account the level of implementation, we are still rigid on one point: components follow standard markup and visuals, DXP cases have different visuals, so the base components are markup oriented, so in a way, we are still rigid and not necessarily a very flexible component. Taking into consideration some of the cases in DXP, comparing Page Elements from Page Editor vs components with In the end being consistent isn't related to the issue of rendering props, it's not the culprit, actually, it's composition, it's hard to limit to a level of block components. Creating high-level is not "usable" for most cases, we can make it cover most of the cases that exist now but a few months from now no more. That would be another thread anyway.
Yes, I agree with you but there's not much to do about it, the case of The implementation is also complicated, so the TreeView is flexible at the definition level, as I said, these are visual and they are more complicated.
@julien Yes, if I can see Vertical Navigation use cases using static, static is actually the low-level look, it's the composite pattern.
This isn't about render props, it's about data. The
Yes, multiple selection and expandable work for both cases, that's not because of render props again, it works like that because it is data-agnostic, is different.
@bryceosterhaus @julien I think in some ways we're trying to blame rendering props for a lot of problems that aren't actually related to it, the problem of being permissive is not the problem of rendering props, the specification is permissive by default, the composition is flexible by default, teams use this to create their own implementations because Lexicon is the foundation, we support that. Static vs dynamic content is not a problem if we can abstract the complexity to be simple to use, we will do it because we have the capability, we have a dedicated team just to think about it but we shouldn't pass that on to the developers. As I've commented at other times the render props allow to be the middle ground, it's not adding more flexibility, in fact, the low-level is already flexible compared to the high-level. We're adding the features that you can't add with the low-level and then we create the high-level. So the problem here is not the render props I think we are not understanding the TreeView implementation level and we might want to impose a lot of limitations. In the end I think all your questions have been answered but I see we are trying to find a problem regarding rendering props that I am not able to understand the reasons for and seems to be unrelated. |
|
There's a lot happening here! Just to give my 2 cents I'd just say that I have the feeling (maybe a wrong one) that we're trying to cover here a lot of stuff and future features instead of creating a basic component for a basic case and iterate over it, which I think is always the best approach IMHO. "We're gonna need this" usually leads to over engineering. So my advice would be:
😘 |
|
I want to add detail so you can think about it, the implementation for TreeView we should consider.
There are a lot of things to think about, so the idea is just to build a foundation that allows all of this, unfortunately being simple with all of this in mind is quite complicated, because we have cases like this so we can't ignore it.
@carloslancha I ended up seeing your comment, Github this time showed it while I was writing 😅. Well actually that's exactly what we're doing 😉, this is an initial sketch that won't be published, it's simple, the other features are to be done in other issues https://github.com/liferay/clay/milestone/46. Thinking about API is exactly what this PR does 😉.
Yes that's always good, in fact this component does exactly that in its first interaction. About Maybe the problem with Like the entire development process, we create, refine, and iterate. |
|
Well, I did a quick API implementation using the low-level and high-level standards, unfortunately, I can't explore everything just with pseudocode maybe the high-level has to have more API to support the full TreeView spec or support the visual of the Page Editor. The markup I saw on the Card is also quite different so it will have to be rewritten anyway because the group is not inside the Item but its sibling. // > Render props 2 in 1
<TreeView
onSelectionChange={(keys) => setSelectionChange(keys)}
selectedKeys={selectedKeys}
expanderIcons={{
close: <Icon symbol="hr" />,
open: <Icon symbol="plus" />,
}}
items={[
{children: [{name: 'Blogs'}], name: 'Liferay Drive'},
{children: [{name: 'Blogs'}], name: 'Repositories'},
{children: [{name: 'PDF'}], name: 'Documents and Media'},
]}
onExpandedChange={(keys) => setExpandedKeys(keys)}
expandedKeys={expandedKeys}
showExpanderOnHover={false}
>
{(item) => (
<TreeView.Item>
{item.name}
<TreeView.Group items={item.children}>
{(item) => (
<TreeView.Item>{item.name}</TreeView.Item>
)}
</TreeView.Group>
</TreeView.Item>
)}
</TreeView>
// --------
// > low-level
<TreeView>
<TreeView.Item>
{'Juan Hidalgo'}
<TreeView.Group>
<TreeView.Item>{'Victor Valle'}</TreeView.Item>
</TreeView.Group>
</TreeView.Item>
</TreeView>
// > high-level
// The APIs will be strict here, we can't be data agnostic for the
// default implementation.
<TreeViewWithItems
onSelectionChange={(keys) => setSelectionChange(keys)}
selectedKeys={selectedKeys}
expanderIcons={{
close: <Icon symbol="hr" />,
open: <Icon symbol="plus" />,
}}
// To render the default behavior we need the API to be strict.
// name
// children
// id
// icon <- This icon is always on the left.
// checkbox: boolean <- We could use selectedKeys and the callback to activate
// this, but I saw that there are cases where some items are not selectable.
//
// The other use cases people would have to use nodeRenderer/itemRenderer
// to create their own markup otherwise we would have to add more APIs
// and the composition of this is in several ways
items={[
{children: [{name: 'Blogs'}], name: 'Liferay Drive'},
{children: [{name: 'Blogs'}], name: 'Repositories'},
{children: [{name: 'PDF'}], name: 'Documents and Media'},
]}
// I think this alone is not enough because of our markup. I have to
// explore more.
// This API may also not need to pass the component reference, but returns
// the component already to avoid adding data in the item structure to be
// visible to the component or using context to do this, the Page Editor
// does that.
// itemRenderer={(item) = {...}}
nodeRenderer={Card}
itemRenderer={Card}
// I'm not sure how this will work just testing to see, the group by markup
// is inside Item, so to cover the different looks and the PageEditor o
// Card case, maybe we need this API too.
groupRenderer={CardGroup}
onExpandedChange={(keys) => setExpandedKeys(keys)}
expandedKeys={expandedKeys}
showExpanderOnHover={false}
/>@bryceosterhaus What do you think? I'll see if I can explore more with the high-level if we can do everything, I'm more concerned about our markup which is too strict so maybe I have to have more APIs to change things. I think the notable difference here is that anyway we have to be permissive to change the markup the visual consistency is not something achievable for all teams, so it has to be flexible. Another point is that using render props is more composition oriented, so it seems simpler to add stuff or remove stuff. |
|
Your examples seem reasonable to me.
💭 One thought, we could have this mid-level be a non-exposed API. Perhaps, we create a mid-level API that we use to compose the high-level components we give to DXP. And hypothetically, we could get months down the road and realize this mid-level is the best option to expose for others and we simply replace the high-levels in place. I liked what @carloslancha had to say too
The more I imagine Carlos outside my window with a threatening gaze, the simpler I want to make things. 🔪 🔪 😱 |
If I understood correctly, we would use this to create the high-level components but it wouldn't be exposed?
I still see a lot of value in exposing low-level APIs they're much more adopted because they cover more use cases, and it's a bit future-proof, taking into account the time, tends to have less change.
Hmm, I'm still not sure if this would be very viable, because in some cases, you have all the basic features, for example, Selectable, DnD, filter, and the default look, others might have all of that with a totally different. I think the problem here is more in allowing the TreeView to be customized to cover Vocabularies, Categories, and Page Elements... there are others that are in the Lexicon spec. With a high-level API difficult to be implemented, it is also very difficult to say what is basic 😅, for example, what exists implemented in DXP is very diverse and also has different visuals, who use the TreeView.Card implemented in React, uses different look than usual and some uses search and selectable, Page Elements uses its own look, own DnD and selectable, some other parts of DXP are AUI but it also has some Card style visuals and some the default look that will be replaced by the new standard component of Lexicon. It still has the implementation of "load more" but it will be available to everyone but I still have no idea how this will reflect in the API.
I would also be concerned if we had the common TreeView for everyone the problem is that visually it's not common, each team has a different look, so that makes the decision more complicated. I still think it won't be difficult, or more flexibly when compared to low-level in a way it's still a bit restricted because we have the low-level which is more flexible than that, so the I think people will be able to use it because it's not a difficult implementation to use, it doesn't add many APIs or more logical complexity the learning curve is very small compared for example to a HOC... I see some occurrences of rendering props via children in DXP, mainly in Forms that we used a lot during migration in
😅 no no, we must avoid this, being more restricted we are harming the development experience and increasing the degree of maintenance and consequently, the components can be depreciated faster, we did this in v2, it didn't work well. I think that instead of creating restricted APIs, we should keep teaching and education of good standards is what will help everyone to evolve with us, keep improving our documentation with more descriptions, examples, everything we write in the examples is used as a reflection in the code and we really should be worrying more about it but it's a big work but I think that's the way to go. At least I can't see something that worked out being restricted maybe even has but your audience can be niched. |
Concerning that, I agree with @bryceosterhaus and I think it's a bit similar to what @carloslancha also mentioned previously. |
|
Hmm Okay, do you have any suggestions on how we could reflect this in the API? or more high-levels? we can start with that since everyone agrees. Remember that creating a very restricted API doesn't mean it's simple and will only hit a few use cases, and what we have in DXP and what's specified by Lexicon are quite different, so maybe just using high-level to cover the different cases, won't help a lot. I don't really like the idea of creating more components for this either. Also, this is very related to my comment on comparing the use of high-level and low-level on DropDown not in terms of whether this is bad practice but how it is used for different use cases. this is getting really long, so I propose we merge that and not expose this API as a primary until we find a better solution or one that covers everything so we don't block the other features. As I also commented in the PR comment and throughout the thread 🙂, this will not be released or exposed until everything is ready, this is just a sketch. |
|
Hey @matuzalemsteles as I said in #4254, having worked on the TreeView component for the drag and drop feature my "worries" about using render props are gone. We'll have to pay attention to devs/teams (as usual) who are new to this, but in the end, it's just another pattern that's available. Concerning the rest of the things I said, I guess hooks might be a bit frightening in the beginning but once you understand the idea, it's the same thing as what I mentioned about render props. Since this isn't going to be released yet, there's nothing to really worry about (and we still might change stuff along the way), so as far as I'm concerned, yes we could merge this. |
Yes, this is very important if this is a primary API, I will definitely be writing documentation with much more detail, is explaining the pattern, and adding the possibilities that you can achieve with it since a lot of code written is a reflection of the documentation.
Yeah, we can have that in Clay's master and maybe it will help us test it on DXP without it's exposed and we can test other alternatives anyway. |
|
Sounds like a good plan @matuzalemsteles. You're also right, testing in DXP should be possible (locally at least), and once we get to that point we will have to decide if we want a tag (but that's for later). |
matuzalemsteles
force-pushed
the
issue-4117
branch
from
1994873 to
8802933
Compare
|
Well, I'm going to go ahead with that and merge this PR and #4222, we're still going to explore other strategies without making render props a primary API yet. Thanks to everyone who raised concerns about this Design. |
Fixes #4117
Well, this is an initial draft for implementation of components for TreeView, the idea is that it looks like a low-level and high-level component, the idea of TreeView is that it is very flexible to build other components like Vertical Navigation or just customize the Node to behave in different variations. An important part of this design is that the TreeView is data agnostic and allows to build a static or dynamic TreeView, we still don't have all the resources because we will be building with other issues.
This component is already being built in
@clayui/corebut it is not being exposed so we are not officially releasing it even if a new version is cut.You can see the stories that have been added with some basic examples.
Design Principles
This component follows some different behaviors when compared to other components but this is already a reflection of the upcoming evolution and refinement of our other components, this can also help with insights for #4112.
Well, this mostly follows our low-level delivery proposal and I'm using a different strategy to balance high-level with low-level and still allow customizations, differently we don't have a component with many APIs or an
itemsthat define the properties, unlike the data here is agnostic and the developer is responsible for assembling theNodesorItemhere according to its data structure and adding new behaviors if is necessary.TreeView uses the Render Props technique to handle different behaviors, static or dynamic values, so we've also been able to abstract some of the complexity that markup adds, as many wrappers to conform to the design spec, we've abstracted here for reduce the amount of components and make the component more "smart".
Static content
Static content is what we normally use for low-level components with just small block composition.
The static component has some different behaviors to allow you to abstract the markup and reduce the writing of more code, for the cases when it is necessary to build a sublevel for the Node it is necessary to add the component
<TreeView.ItemStack />explicitly to create the Node If the Node has more information besides Text, another component can also be used.For cases where you only have the Node text, you don't need to explicitly add the
<TreeView.ItemStack />.Dynamic content
Dynamic content will be used very often, and it's usually when we create the high-level component to solve this problem, differently we are adopting the Render Props technique to create the balance between low-level and high-level and still allow to customize the elements.
Unlike the other components we are not defining a data structure that must be followed, this is complicated because normally we would force everyone to use this structure and adapt a good part of the products to deal with it, being agnostic also reduced our API surface.
The properties of
itemsalong with functions onchildrenare what allow the<TreeView />and<TreeView.Group />component to render the component and pass the correspondingitemto the iterable flow. In addition to handling this, the component abstracts the markups that are needed to render correctly to prevent the developer from adding them explicitly.Accessibility
This is an early implementation so we haven't dealt with Expanding Nodes or Multiple Selection that will be worked on in #4210 yet, as TreeView will take care of this business rule we will also be controlling accessibility properties like
aria-expanded,aria-controls...