-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: tree component #566
feat: tree component #566
Conversation
42a48d3
to
77c23dd
Compare
shouldItemFocusBeInset?: boolean; | ||
|
||
/** | ||
* Determines if the tree node is interactive (usually used as a header when false) |
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.
What do you mean with '(usually used as a header when false)' ? If the node is not interactive, it is probably a leaf node right?
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.
good point. This was a leftover from the ListItemBase, but we do not really need this
I removed the interactive
and isDisabled
props for now because if we can disable nodes then it will effect on the navigation.
Let's postpone this functionality until we really need it
...rest | ||
} = props; | ||
if (!nodeId) { | ||
throw new Error('TreeNodeBase: nodeId prop is required.'); |
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.
// ref directly because we want it to be a useRef style ref | ||
// We useLayoutEffect so that it happens in time for tippy | ||
// to use the ref when adding event handlers | ||
useLayoutEffect(() => { |
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.
Is this for the case of having a tree component inside a popover? Do we have a use case for that? I can only think of calendar item popover, but curious if there is any other
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.
Since this solution works in list and you already mentioned a possible use case, I keep this code
content = children; | ||
} else { | ||
console.warn( | ||
'TreeNodeBase: this component can only receive ListItemBaseSection as children.' |
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.
can only receive ListItemBaseSection or Text as children
} else { | ||
console.warn( | ||
'TreeNodeBase: this component can only receive ListItemBaseSection as children.' | ||
); |
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.
So, if children is an array, the only permitted cases are:
[<ListItemBaseSection />, <ListItemBaseSection/ >, <ListItemBaseSection>]
[<ListItemBaseSection />, <ListItemBaseSection/ >]
[<Text />, <Text/ >, <Text>]
[<Text />, <Text/ >]
Why do we want to make this that restrictive? why not simply rendering the children inside the node?
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 re-phrased the warning.
Valid children are:
- Single, eg.:
<div/>
,</>
(React.Frament) - Multiple eg: `[, <Text/ >]
The message is misleading, [<ListItemBaseSection />, <ListItemBaseSection/ >]
is not allowed, instead you have to use: [<ListItemBaseSection />]
and put the multiple child into ListItemBaseSection
*/ | ||
const shouldFocusOnPress = treeContext?.shouldFocusOnPress || false; | ||
const shouldItemFocusBeInset = | ||
treeContext?.shouldItemFocusBeInset || DEFAULTS.SHOULD_ITEM_FOCUS_BE_INSET; |
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.
On TreeNodeBase.tsx I see there is a prop called shouldItemFocusBeInset. Why are we not using this for this line too?
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.
Good point, we do not need the shouldItemFocusBeInset
on the TreeNodeBase, removed
(also renamed it in the Tree component's porps)
const shouldItemFocusBeInset = | ||
treeContext?.shouldItemFocusBeInset || DEFAULTS.SHOULD_ITEM_FOCUS_BE_INSET; | ||
|
||
const treeNodeTabIndex = getTreeNodeBaseTabIndex(nodeId, treeContext.activeNodeId, interactive); |
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.
shouldnt this const be called treeNodeBaseTabIndex?
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.
Since it is a private variable of the component, the prefix is not needed. I reduced the name to tabIndex
only
src/utils/navigation.ts
Outdated
const FOCUSABLE_ELEMENT_SELECTORS = | ||
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex=""]'; | ||
const FOCUSABLE_AND_TABBABLE_ONLY_ELEMENT_SELECTORS = | ||
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex="-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.
const FOCUSABLE_ELEMENT_SELECTORS = | |
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex=""]'; | |
const FOCUSABLE_AND_TABBABLE_ONLY_ELEMENT_SELECTORS = | |
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex="-1"]'; | |
const FOCUSABLE_ELEMENT_SELECTORS = | |
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex=""])'; | |
const FOCUSABLE_AND_TABBABLE_ONLY_ELEMENT_SELECTORS = | |
'a[href], button, input, textarea, select, details, [tabindex]:not([tabindex="-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.
Nice catch!
*/ | ||
export const createTreeNode = ( | ||
id: TreeNodeId, | ||
isOpenByDefault: boolean = undefined, |
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.
isn't this the same as to say isOpenByDefault?: boolean
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.
do we need the default to undefined?
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.
it is the same as isOpenByDefault?: boolean
but it is more explicit this way, and following the patter in the next line children: Array<TreeNode> = []
src/components/Tree/Tree.utils.ts
Outdated
return value; | ||
}; | ||
|
||
const TREE_NAVIGATION_ITERATION_LIMIT = 1_024; |
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.
this should be in constants file
src/components/Tree/Tree.utils.ts
Outdated
* @param tree | ||
* @param nodeId | ||
*/ | ||
export const toggleActiveNode = (tree: FlatTree, nodeId: TreeNodeId): TreeNodeId => { |
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 name of this util is confusing, it doesn't get the active node, it uses the passed nodeid and toggles it. So maybe a name like toggleNode
would be better
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.
Actually we do not event need this function
src/components/Tree/Tree.utils.ts
Outdated
*/ | ||
export const toggleActiveNode = (tree: FlatTree, nodeId: TreeNodeId): TreeNodeId => { | ||
const node = tree.get(nodeId); | ||
if (node) { |
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.
should we log any warning if the node doesn't exist?
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 added a check to getNextActiveNode
, not sure about the rest of the case. Some time is valid to have an undefined result for example when we reach the root which has not parent
If the `<TreeNodeBase />` can not access ot the context, it will throw an error. | ||
|
||
`<TreeNodeBase />` allows to navigate into the node itself when there are interactive elements inside it. | ||
After the node selected the user can use the `Tab` key to select the first, second, etc. interactive element. |
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.
'After the node is focused (not selected), the user can ...'
|
||
`<TreeNodeBase />` allows to navigate into the node itself when there are interactive elements inside it. | ||
After the node selected the user can use the `Tab` key to select the first, second, etc. interactive element. | ||
When the last interactable element is selected, the next tab will move the focus to the next interactable |
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.
'When the last interactable element is focused, the next tab will move the focus to the next interactable...'
src/components/Tree/Tree.tsx
Outdated
'aria-setsize': isRoot ? 1 : parent.children.length, | ||
'aria-level': node.level + 1, | ||
'aria-posinset': node.index + 1, | ||
'aria-selected': isSelected, |
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 shouldn't set aria-selected if activeNode === node.id (that is, if the node is simply focused).
src/components/Tree/Tree.utils.ts
Outdated
|
||
// Otherwise, find the next sibling | ||
for (let counter = 0; ; counter++) { | ||
if (counter > TREE_NAVIGATION_ITERATION_LIMIT) { |
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.
question: we do support 1K participants, and if we're gonna use this in the participants list, we might reach the limit.. but not sure how realistic that is.
Any particular reason you added it or?
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.
it does not limit the size of the tree, it is to detect loops in the tree.
I will write some test cases.
Also I refactored it :)
* @param excludeRoot | ||
* @param toggleTreeNode | ||
*/ | ||
export const getNextActiveNode = ( |
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.
suggestion: maybe rename this func, there's another function above findNextTreeNode
, and in the comment it says * Find the next active tree node based on the current active node
. I can see that the imlpementation of this one is actually different, it actually handles the keyboard nav.
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.
findNextTreeNode
is just a utility function which is not exported and used only in the getNextActiveNode
function. I can update the docs to make it more descriptive, but I am also open for name suggestions
src/components/Tree/Tree.utils.ts
Outdated
}; | ||
|
||
/** | ||
* Travers the tree and convert it to a map between the node id and the node |
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.
* Travers the tree and convert it to a map between the node id and the node | |
* Traverse the tree and convert it to a map between the node id and the node |
* | ||
* It is used to build an internal tree for navigation and follow open/close states. | ||
*/ | ||
treeStructure: TreeRoot; |
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.
TreeRoot is of type TreeNode, which does not have a flat structure. What is we want a flat structure?
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.
flat tree is the internal representation of the tree inside the Tree component, it is more efficient to use the flattened tree for navigation then the normal nested tree.
Please note, it is independent from the isRenderedFlat
because it is about how the consumer built the tree in the DOM
The consumer should not know about this internal representation.
/** | ||
* Toggle open/close state of the tree node. | ||
*/ | ||
setVirtualTreeNodeOpenState?: (id: TreeNodeId, isOpen: boolean) => void; |
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.
you have a type set for this called ToggleTreeNode
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.
Unfortunately there are different. The isOpen
parameter is option in the ToggleTreeNode
and required in setVirtualTreeNodeOpenState
src/components/Tree/Tree.types.ts
Outdated
* Toggle the isOpen state of the tree node. | ||
* @param id unique identifier of the tree node | ||
*/ | ||
toggleTreeNode: (id: TreeNodeId) => void; |
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 have a type called ToggleTreeNode that is a function with id and isOpen as params. It is confusing that one toggleTreeNode only needs id and the other needs both id and isOpen stated, don't you think?
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.
some small issues still, but looking great otherwise
src/components/Tree/Tree.types.ts
Outdated
* we need to provide additional aria attributes to re-build it for the accessibility tree. | ||
* Virtualized trees are usually rendered flat. | ||
* | ||
* @see {@link https://codepen.io/maxinteger/pen/zYVNabV Example of a nested and flat tree} |
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're still using codepen, but I'm not going to block it for that.
src/components/Tree/Tree.utils.ts
Outdated
const loopCheck = new Set<TreeNodeId>(); | ||
|
||
// Otherwise, find the next sibling | ||
for (;;) { |
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.
suggestion: while (true) {}
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.
ESLint does not like it, no-constant-condition
, I do it anyway, while(true)
is more readable then for(;;)
src/components/Tree/Tree.utils.ts
Outdated
if (!current.isLeaf) { | ||
if (!current.isOpen) { | ||
// Open it if it's closed | ||
|
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.
nitpick: unnecessary new line
* @param prevTree | ||
* @param isOpen | ||
*/ | ||
export const toggleTreeNodeRecord = ( |
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.
question: what is the difference between this and toggleTreeNode
from
const toggleTreeNode = useCallback( |
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.
in toggleTreeNodeRecord
the isOpen
parameter required while in the toggleTreeNode
is optional. Also, toggleTreeNodeRecord
is used internally meantime toggleTreeNode
is part of the Tree context API.
I flagged toggleTreeNodeRecord
as @internal
box-sizing: border-box; | ||
outline: none; | ||
|
||
&[data-interactive='true'] { |
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.
issue: you're missing interactive prop on the tree node base (you need to follow similar approach as we do in list item base), OR just remove it? Not sure we need interactive
for tree node... but you never know..
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.
Nice catch, Tree node is interactive by default, at least for now. I removed the &[data-interactive='true'] {
, but kept the rest.
box-sizing: border-box; | ||
outline: none; | ||
|
||
&[data-interactive='true'] { |
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.
issue: I don't see any disabled state
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've identified both of this issue from the storybook page of this component. I noticed you've used the template for pseudo states (praise!), and they're broken. The reason they are broken are because of the 2 issues abvove.
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.
yes, there is no disabled state fro the tree node at the moment. We can add it when it is needed.
data-shape={shape} | ||
className={classnames(className, STYLE.wrapper, { active: isPressed || isSelected })} | ||
lang={lang} | ||
{...treeNodePressProps} |
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.
suggestion: we should allow the pass in the ..rest
props to this as long as they're valid tags. Not sure what the best approach for this is.. We normally just pass in ...rest BUT I've noticed a lot in Cantina and mrv2 that a lot of times we pass in invalid DOM attributes to the HMTL elements, which result in warnings in unit test output and console.logs in browser.
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.
This is helpful if we need to pass any aria attributes from the consumer side.
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.
fixed
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.
some small issues still, but looking great otherwise
f25a2bd
to
0023a27
Compare
🎉 This PR is included in version 26.143.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add new Tree and TreeBaseNode components
Links
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-548345