fix: pr comments VIII

momentum-design · Aug 22, 2024 · f25a2bd · f25a2bd
1 parent 44edce1
commit f25a2bd
Show file tree

Hide file tree

Showing 6 changed files with 152 additions and 25 deletions.
diff --git a/src/components/Tree/Tree.stories.docs.mdx b/src/components/Tree/Tree.stories.docs.mdx
@@ -6,4 +6,128 @@ make it possible to use it with Virtualized trees.
 
 Tree nodes must be wrapped in a `<TreeNodeBase />` component to ensure proper keyboard navigation and focus management.
 
-Any node inside the `<Tree />` can access to the tree context using the `useTreeContext` hook.
+Any node inside the `<Tree />` can access to the tree context using the `useTreeContext` hook.
+
+## Nested VS Flat tree
+
+There is 2 ways to represent the tree structure in the DOM:
+
+### 1) Nested elements
+
+Simple static trees usually represented as nested elements. It's easy to understand and implement and
+most of the semantics are implicitly set. But hard to render it automatically especially in virtualized trees.
+
+```html
+<ul role="tree" aria-labelledby="tree_label">
+    <li role="treeitem" aria-expanded="false" aria-selected="false">
+        <span> Level 1 </span>
+        <ul role="group">
+            <li role="treeitem" aria-selected="false">Level 2.1</li>
+            <li role="treeitem" aria-selected="false">Level 2.2</li>
+            <li role="treeitem" aria-selected="false">Level 2.3</li>
+            <li role="treeitem" aria-expanded="false" aria-selected="false">
+                <span> Level 2.4 </span>
+                <ul role="group">
+                    <li role="treeitem" aria-selected="false">Level 3.1</li>
+                    <li role="treeitem" aria-selected="false">Level 3.2</li>
+                </ul>
+            </li>
+            <li role="treeitem" aria-selected="false">Level 2.5</li>
+        </ul>
+    </li>
+</ul>
+
+```
+
+### 2) Flat elements
+
+Flat tree structure is more flexible and can be used with virtualized trees. It requires more explicit semantics.
+
+```html
+<h3 id="tree_label_2">Tree with flat DOM</h3>
+<ul role="tree" aria-labelledby="tree_label_2">
+    <li
+            role="treeitem"
+            aria-level="1"
+            aria-expanded="true"
+            aria-selected="false"
+            class="level-1"
+    >
+        <span> Level 1 </span>
+        <div role="group" aria-owns="level-2.1 level-2.2 level-2.3 level-2.4 level-2.5"></div>
+    </li>
+    <li
+            id="level-2.1"
+            aria-setsize="5"
+            aria-posinset="1"
+            role="treeitem"
+            aria-level="2"
+            class="level-2"
+    >
+        <span>  Level 2.1</span>
+    </li>
+    <li
+            id="level-2.2"
+            aria-setsize="5"
+            aria-posinset="2"
+            role="treeitem"
+            aria-level="2"
+            class="level-2"
+    >
+        <span>  Level 2.2</span>
+    </li>
+    <li
+            id="level-2.3"
+            aria-setsize="5"
+            aria-posinset="3"
+            role="treeitem"
+            aria-level="2"
+            class="level-2"
+    >
+        <span>  Level 2.3</span>
+    </li>
+    <li
+            id="level-2.4"
+            aria-setsize="5"
+            aria-posinset="4"
+            role="treeitem"
+            aria-level="2"
+            class="level-2"
+            aria-expanded="true"
+
+    >
+        Level 2.4
+        <div role="group" aria-owns="level-3.1 level-3.2"></div>
+    </li>
+    <li
+            id="level-3.1"
+            aria-setsize="2"
+            aria-posinset="1"
+            role="treeitem"
+            aria-level="3"
+            class="level-3"
+    >
+        <span>  Level 3.1</span>
+    </li>
+    <li
+            id="level-3.2"
+            aria-setsize="2"
+            aria-posinset="2"
+            role="treeitem"
+            aria-level="3"
+            class="level-3"
+    >
+        <span>  Level 3.2</span>
+    </li>
+    <li
+            id="level-2.5"
+            aria-setsize="5"
+            aria-posinset="5"
+            role="treeitem"
+            aria-level="2"
+            class="level-2"
+    >
+        <span>  Level 2.5</span>
+    </li>
+</ul>
+```
diff --git a/src/components/Tree/Tree.types.ts b/src/components/Tree/Tree.types.ts
@@ -1,4 +1,4 @@
-import { CSSProperties, HTMLAttributes, ReactNode } from 'react';
+import { CSSProperties, DetailedHTMLProps, HTMLAttributes, ReactNode } from 'react';
 
 /**
  * The key codes used to navigate the tree.
@@ -85,7 +85,7 @@ export type TreeIdNodeMap = Map<TreeNodeId, TreeNodeRecord>;
 /**
  * The props of the Tree component.
  */
-export interface Props {
+export interface Props extends DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement> {
   /**
    * Custom class for overriding this component's CSS.
    */
@@ -125,8 +125,6 @@ export interface Props {
    * 2) Flat: The tree is rendered as a single level list where the DOM does not reflect the semantic structure of the tree, and
    *    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}
    * @default true
    */
   isRenderedFlat?: boolean;

diff --git a/src/components/Tree/Tree.utils.ts b/src/components/Tree/Tree.utils.ts
@@ -36,6 +36,7 @@ export const getTreeRootId = (tree: TreeIdNodeMap): TreeNodeId | undefined => {
  * Find the next active tree node based on the current active node
  * @param tree
  * @param activeNodeId
+ * @internal
  */
 const findNextTreeNode = (tree: TreeIdNodeMap, activeNodeId: TreeNodeId): TreeNodeId => {
   let current = tree.get(activeNodeId);
@@ -48,7 +49,8 @@ const findNextTreeNode = (tree: TreeIdNodeMap, activeNodeId: TreeNodeId): TreeNo
   const loopCheck = new Set<TreeNodeId>();
 
   // Otherwise, find the next sibling
-  for (;;) {
+  // eslint-disable-next-line no-constant-condition
+  while (true) {
     const parent = tree.get(current.parent);
     const pos = current.index + 1;
 
@@ -75,6 +77,7 @@ const findNextTreeNode = (tree: TreeIdNodeMap, activeNodeId: TreeNodeId): TreeNo
  * @param tree
  * @param excludeRootNode
  * @param activeNodeId
+ * @internal
  */
 const findPreviousTreeNode = (
   tree: TreeIdNodeMap,
@@ -119,6 +122,7 @@ const findPreviousTreeNode = (
  * @param tree
  * @param activeNodeId
  * @param toggleTreeNode
+ * @internal
  */
 const openNextNode = (
   tree: TreeIdNodeMap,
@@ -130,7 +134,6 @@ const openNextNode = (
   if (!current.isLeaf) {
     if (!current.isOpen) {
       // Open it if it's closed
-
       toggleTreeNode(activeNodeId);
       return activeNodeId;
     } else {
@@ -149,6 +152,7 @@ const openNextNode = (
  * @param activeNodeId
  * @param excludeRoot
  * @param toggleTreeNode
+ * @internal
  */
 const closeNextNode = (
   tree: TreeIdNodeMap,
@@ -280,6 +284,7 @@ export const getNextActiveNode = (
  * @param id
  * @param prevTree
  * @param isOpen
+ * @internal
  */
 export const toggleTreeNodeRecord = (
   id: TreeNodeId,

diff --git a/src/components/TreeNodeBase/TreeNodeBase.style.scss b/src/components/TreeNodeBase/TreeNodeBase.style.scss
@@ -9,24 +9,21 @@
   transition: background-color 0.2s, color 0.2s, border-color 0.2s;
   box-sizing: border-box;
   outline: none;
+  cursor: pointer;
 
-  &[data-interactive='true'] {
-    cursor: pointer;
-
-    &:hover,
-    &.hover {
-      background-color: var(--mds-color-theme-background-primary-hover);
-    }
+  &:hover,
+  &.hover {
+    background-color: var(--mds-color-theme-background-primary-hover);
+  }
 
-    &:active,
-    &.active {
-      color: var(--mds-color-theme-text-primary-normal);
-      background-color: var(--mds-color-theme-background-primary-active);
-    }
+  &:active,
+  &.active {
+    color: var(--mds-color-theme-text-primary-normal);
+    background-color: var(--mds-color-theme-background-primary-active);
+  }
 
-    &.focus {
-      color: var(--mds-color-theme-text-primary-normal);
-    }
+  &.focus {
+    color: var(--mds-color-theme-text-primary-normal);
   }
 
   & > div[data-position='start'] {

diff --git a/src/components/TreeNodeBase/TreeNodeBase.tsx b/src/components/TreeNodeBase/TreeNodeBase.tsx
@@ -108,7 +108,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
   const { pressProps, isPressed } = usePress({
     preventFocusOnPress: true, // we handle it ourselves
     onPress: internalOnPress,
-    ...rest,
+    ref,
   });
 
   // Prevent tree node update because it can cause state lost in the focused component e.g. Menu
@@ -185,6 +185,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
         lang={lang}
         {...treeNodePressProps}
         {...treeContext.getNodeProps(nodeId)}
+        {...rest}
       >
         {content}
         {treeContext.isRenderedFlat && !isLeaf && (

diff --git a/src/components/TreeNodeBase/TreeNodeBase.types.ts b/src/components/TreeNodeBase/TreeNodeBase.types.ts
@@ -1,4 +1,4 @@
-import { CSSProperties, ReactNode, RefObject } from 'react';
+import { CSSProperties, DetailedHTMLProps, HTMLAttributes, ReactNode, RefObject } from 'react';
 import { PressEvents } from '@react-types/shared';
 import { TreeNodeId } from '../Tree';
 
@@ -17,7 +17,9 @@ export type TreeNodeBaseRefOrCallbackRef =
 /**
  * TreeNodeBase Props
  */
-export interface Props extends PressEvents {
+export interface Props
+  extends PressEvents,
+    DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement> {
   /**
    * Unique identifier for the tree node
    */