-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Tree): control activeItemIds through expanded prop of items #2061
Conversation
Let's add an example of how the user can control the expended prop |
@@ -117,7 +117,7 @@ class TreeItem extends UIComponent<WithAsProp<TreeItemProps>, TreeItemState> { | |||
onFocusFirstChild: PropTypes.func, | |||
onFocusParent: PropTypes.func, | |||
onSiblingsExpand: PropTypes.func, | |||
open: PropTypes.bool, | |||
expanded: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be treated as auto controlled prop, by defining defaultExpanded
and onExpandedChange
. With this we will allow the user to define only the initial state, if they want that, or listen to the onExpandedChange
and change the expanded
prop by themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not having any state in TreeItem. All expanded stateful logic is in Tree, since TreeItems can be unmounted on virtualization. This is just a prop. The user can control it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this one. I understand that it is just a prop, but all other similar props are treated as autocontrolled. @miroslavstastny thoughts on this?
_.reduce( | ||
items, | ||
(acc, item) => { | ||
if (item['expanded'] && acc.indexOf(item['id']) === -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The items could be defined as render functions too, we should handle that case, or at least check whether it is an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it won't work well with render functions. We are calculating some props that we pass to TreeItem from Tree in getItemsForRender
. However we probably can support this scenario with render prop as well. We just call the children function with the props from itemsForRender
so the child component has access to them as well.
I would consider working on this in a separate PR. And just perform here the object check as per suggestion.
What do you think @mnajdova @jurokapsiar @miroslavstastny ?
User passing
expanded
prop to items will allow him to control the expanded state of these items. Renamedopen
toexpanded
.Controlling the Tree's
activeItemIds
will have precedence over this individual control method.Added also missing tests for Tree, subcomponents and behaviors. Added / edited test definitions for behavior tests.