Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Tree): control activeItemIds through expanded prop of items #2061

Merged
merged 22 commits into from
Nov 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8ae5d5c
add expanded ids in activeItemIds
silviuaavram Oct 24, 2019
90e08e0
replace open with expanded
silviuaavram Oct 24, 2019
369a18a
made test to work for expanded
silviuaavram Oct 24, 2019
632ac1f
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Oct 24, 2019
27d6bef
fixed issue and added tests
silviuaavram Oct 25, 2019
084ff63
Merge branch 'feat/control-tree-expanded' of https://github.com/stard…
silviuaavram Oct 25, 2019
f0b7d8c
changelog
silviuaavram Oct 25, 2019
af2685d
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Oct 25, 2019
78f1087
fix changelog
silviuaavram Oct 25, 2019
1cf0ba8
fix changelog again
silviuaavram Oct 25, 2019
b35bc5d
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Oct 29, 2019
09a9adf
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Nov 8, 2019
4d8a16c
add test to behavior
silviuaavram Nov 11, 2019
c2aa1e1
fix expanded issue
silviuaavram Nov 11, 2019
13608d5
add test to tree
silviuaavram Nov 11, 2019
b464fa2
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Nov 11, 2019
6c3cf66
add more tests to TreeItem
silviuaavram Nov 11, 2019
f28dbce
more tests to behavior
silviuaavram Nov 12, 2019
c978b34
TreeTitle conformant
silviuaavram Nov 12, 2019
93213ce
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Nov 12, 2019
b7a822f
default size is actually 1
silviuaavram Nov 12, 2019
6b55208
Merge branch 'master' into feat/control-tree-expanded
silviuaavram Nov 15, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### BREAKING CHANGES
- Control Tree `activeItemIds` through `expanded` TreeItem prop @silviuavram ([#2061](https://github.com/stardust-ui/react/pull/2061))

### Fixes
- Update Silver color scheme, changing `backgroundHover` and `backgroundPressed` for high-contrast theme @pompomon ([#2078](https://github.com/microsoft/fluent-ui-react/pull/2078))
Expand Down
30 changes: 16 additions & 14 deletions packages/accessibility/src/behaviors/Tree/treeItemBehavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ import { IS_FOCUSABLE_ATTRIBUTE } from '../../attributes'
import treeTitleBehavior from './treeTitleBehavior'

/**
* @description
* Adds role 'treeitem' to a non-leaf item and 'none' to a leaf item.
* Adds 'aria-expanded' with a value based on the 'open' prop if item is not a leaf.
* Adds 'tabIndex' as '-1' if the item is not a leaf.
*
* @specification
* Adds attribute 'aria-expanded=true' based on the property 'expanded' if the component has 'hasSubtree' property.
* Adds attribute 'tabIndex=-1' to 'root' slot if 'hasSubtree' property is true. Does not set the attribute otherwise.
* Adds attribute 'aria-setsize=3' based on the property 'treeSize' if the component has 'hasSubtree' property.
* Adds attribute 'aria-posinset=2' based on the property 'index' if the component has 'hasSubtree' property.
* Adds attribute 'aria-level=1' based on the property 'level' if the component has 'hasSubtree' property.
* Adds attribute 'role=treeitem' to 'root' slot if 'hasSubtree' property is true. Sets the attribute to 'none' otherwise.
* Triggers 'performClick' action with 'Enter' or 'Spacebar' on 'root'.
* Triggers 'expandSiblings' action with '*' on 'root'.
* Triggers 'focusParent' action with 'ArrowLeft' on 'root', when has a closed subtree.
* Triggers 'collapse' action with 'ArrowLeft' on 'root', when has an opened subtree.
* Triggers 'expand' action with 'ArrowRight' on 'root', when has a closed subtree.
Expand All @@ -22,7 +24,7 @@ const treeItemBehavior: Accessibility<TreeItemBehaviorProps> = props => ({
root: {
role: 'none',
...(props.hasSubtree && {
'aria-expanded': props.open,
'aria-expanded': props.expanded,
tabIndex: -1,
[IS_FOCUSABLE_ATTRIBUTE]: true,
role: 'treeitem',
Expand All @@ -37,15 +39,15 @@ const treeItemBehavior: Accessibility<TreeItemBehaviorProps> = props => ({
performClick: {
keyCombinations: [{ keyCode: keyboardKey.Enter }, { keyCode: keyboardKey.Spacebar }],
},
...(isSubtreeOpen(props) && {
...(isSubtreeExpanded(props) && {
collapse: {
keyCombinations: [{ keyCode: keyboardKey.ArrowLeft }],
},
focusFirstChild: {
keyCombinations: [{ keyCode: keyboardKey.ArrowRight }],
},
}),
...(!isSubtreeOpen(props) && {
...(!isSubtreeExpanded(props) && {
expand: {
keyCombinations: [{ keyCode: keyboardKey.ArrowRight }],
},
Expand All @@ -64,18 +66,18 @@ const treeItemBehavior: Accessibility<TreeItemBehaviorProps> = props => ({
})

export type TreeItemBehaviorProps = {
/** If item is a subtree, it indicates if it's open. */
open?: boolean
/** If item is a subtree, it indicates if it's expanded. */
expanded?: boolean
level?: number
index?: number
hasSubtree?: boolean
treeSize?: number
}

/** Checks if current tree item has a subtree and it is opened */
const isSubtreeOpen = (props: TreeItemBehaviorProps): boolean => {
const { hasSubtree, open } = props
return !!(hasSubtree && open)
/** Checks if current tree item has a subtree and it is expanded */
const isSubtreeExpanded = (props: TreeItemBehaviorProps): boolean => {
const { hasSubtree, expanded } = props
return !!(hasSubtree && expanded)
}

export default treeItemBehavior
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import { IS_FOCUSABLE_ATTRIBUTE } from '../../attributes'
import { Accessibility } from '../../types'

/**
* @description
* Adds role 'treeitem' if the title is a leaf node inside the tree.
* Adds 'tabIndex' as '-1' if the title is a leaf node inside the tree.
*
* @specification
* Adds attribute 'tabIndex=-1' to 'root' slot if 'hasSubtree' property is false or undefined. Does not set the attribute if true.
* Adds attribute 'role=treeitem' to 'root' slot if 'hasSubtree' property is false or undefined. Does not set the attribute if true.
* Adds attribute 'aria-setsize=3' based on the property 'treeSize' if the component has 'hasSubtree' property false or undefined. Does not set anything if true..
* Adds attribute 'aria-posinset=2' based on the property 'index' if the component has 'hasSubtree' property false or undefined. Does not set anything if true..
* Adds attribute 'aria-level=1' based on the property 'level' if the component has 'hasSubtree' property false or undefined. Does not set anything if true..
* Triggers 'performClick' action with 'Enter' or 'Spacebar' on 'root'.
*/
const treeTitleBehavior: Accessibility<TreeTitleBehavior> = props => ({
Expand Down
103 changes: 89 additions & 14 deletions packages/accessibility/test/behaviors/testDefinitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,13 @@ function testMethodConditionallyAddAttribute(
component,
propertyDependsOn,
valueOfProperty,
valueOfPropertyOtherwise,
attributeToBeAdded,
valueOfAttributeToBeAddedIfTrue,
valueOfAttributeToBeAddedOtherwise,
) {
const propertyWithAriaSelected = {}
propertyWithAriaSelected[propertyDependsOn] = valueOfPropertyOtherwise
silviuaavram marked this conversation as resolved.
Show resolved Hide resolved
const expectedResultAttributeNotDefined = parameters.behavior(propertyWithAriaSelected)
.attributes[component][attributeToBeAdded]
expect(testHelper.convertToMatchingTypeIfApplicable(expectedResultAttributeNotDefined)).toEqual(
Expand All @@ -189,7 +191,7 @@ function testMethodConditionallyAddAttribute(

// Example: Adds attribute 'aria-disabled=true' to 'trigger' slot if 'disabled' property is true. Does not set the attribute otherwise.
definitions.push({
regexp: /Adds attribute '([\w-]+)=([\w\d]+)' to '([\w-]+)' slot if '([\w-]+)' property is true\. Does not set the attribute otherwise\./g,
regexp: /Adds attribute '([\w-]+)=([\w\d-]+)' to '([\w-]+)' slot if '([\w-]+)' property is true\. Does not set the attribute otherwise\./g,
testMethod: (parameters: TestMethod) => {
const [
attributeToBeAdded,
Expand All @@ -203,6 +205,31 @@ definitions.push({
component,
propertyDependsOn,
true,
undefined,
attributeToBeAdded,
valueOfAttributeToBeAdded,
undefined,
)
},
})

// Example: Adds attribute 'aria-disabled=true' to 'trigger' slot if 'disabled' property is false or undefined. Does not set the attribute if true.
definitions.push({
regexp: /Adds attribute '([\w-]+)=([\w\d-]+)' to '([\w-]+)' slot if '([\w-]+)' property is false or undefined\. Does not set the attribute if true\./g,
testMethod: (parameters: TestMethod) => {
const [
attributeToBeAdded,
valueOfAttributeToBeAdded,
component,
propertyDependsOn,
] = parameters.props

testMethodConditionallyAddAttribute(
parameters,
component,
propertyDependsOn,
undefined,
true,
attributeToBeAdded,
valueOfAttributeToBeAdded,
undefined,
Expand All @@ -227,14 +254,15 @@ definitions.push({
component,
propertyDependsOn,
true,
undefined,
attributeToBeAdded,
valueOfAttributeToBeAddedIfTrue,
valueOfAttributeToBeAddedOtherwise,
)
},
})

// Adds attribute 'aria-haspopup=true' to 'root' slot if 'menu' menu property is set.
// Adds attribute 'aria-haspopup=true' to 'root' slot if 'menu' property is set.
definitions.push({
regexp: /Adds attribute '([\w-]+)=([\w\d]+)' to '([\w-]+)' slot if '([\w-]+)' property is set\./g,
testMethod: (parameters: TestMethod) => {
Expand All @@ -251,6 +279,7 @@ definitions.push({
component,
propertyDependsOn,
'custom-value',
undefined,
attributeToBeAdded,
valueOfAttributeToBeAddedIfTrue,
valueOfAttributeToBeAddedOtherwise,
Expand Down Expand Up @@ -317,6 +346,44 @@ definitions.push({
},
})

// Example: Adds attribute 'aria-expanded=true' based on the property 'open' if the component has 'hasSubtree' property false or undefined. Does not set anything if true.
definitions.push({
regexp: /Adds attribute '([\w-]+)=(\w+)' based on the property '(\w+)' if the component has '(\w+)' property false or undefined. Does not set anything if true\./g,
testMethod: (parameters: TestMethod) => {
const [
attributeToBeAdded,
attributeExpectedValue,
propertyDependingOnFirst,
propertyDependingOnSecond,
] = parameters.props

const property = {}

property[propertyDependingOnFirst] = attributeExpectedValue
property[propertyDependingOnSecond] = false
const actualResultIfFalse = parameters.behavior(property).attributes.root[attributeToBeAdded]
expect(testHelper.convertToMatchingTypeIfApplicable(actualResultIfFalse)).toEqual(
testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue),
)

property[propertyDependingOnSecond] = undefined
const actualResultIfUndefined = parameters.behavior(property).attributes.root[
attributeToBeAdded
]
expect(testHelper.convertToMatchingTypeIfApplicable(actualResultIfUndefined)).toEqual(
testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue),
)

const propertyFirstPropUndefined = {}
propertyFirstPropUndefined[propertyDependingOnSecond] = true
const actualResultFirstPropertyNegateUndefined = parameters.behavior(propertyFirstPropUndefined)
.attributes.root[attributeToBeAdded]
expect(
testHelper.convertToMatchingTypeIfApplicable(actualResultFirstPropertyNegateUndefined),
).toEqual(undefined)
},
})

// Example: Adds attribute 'aria-expanded=true' based on the property 'open' if the component has 'hasSubtree' property.
definitions.push({
regexp: /Adds attribute '([\w-]+)=(\w+)' based on the property '(\w+)' if the component has '(\w+)' property\./g,
Expand All @@ -337,16 +404,18 @@ definitions.push({
testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue),
)

const propertyFirstPropNegate = {}
propertyFirstPropNegate[
propertyDependingOnFirst
] = !testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue)
propertyFirstPropNegate[propertyDependingOnSecond] = true
const actualResultFirstPropertyNegate = parameters.behavior(propertyFirstPropNegate).attributes
.root[attributeToBeAdded]
expect(testHelper.convertToMatchingTypeIfApplicable(actualResultFirstPropertyNegate)).toEqual(
!testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue),
)
if (typeof testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue) === 'boolean') {
silviuaavram marked this conversation as resolved.
Show resolved Hide resolved
const propertyFirstPropNegate = {}
propertyFirstPropNegate[
propertyDependingOnFirst
] = !testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue)
propertyFirstPropNegate[propertyDependingOnSecond] = true
const actualResultFirstPropertyNegate = parameters.behavior(propertyFirstPropNegate)
.attributes.root[attributeToBeAdded]
expect(testHelper.convertToMatchingTypeIfApplicable(actualResultFirstPropertyNegate)).toEqual(
!testHelper.convertToMatchingTypeIfApplicable(attributeExpectedValue),
)
}

const propertyFirstPropUndefined = {}
propertyFirstPropUndefined[propertyDependingOnFirst] = true
Expand Down Expand Up @@ -597,7 +666,13 @@ definitions.push({
regexp: /Triggers '(\w+)' action with '(\w+)' on '([\w-]+)', when has an opened subtree\./g,
testMethod: (parameters: TestMethod) => {
const [action, key, elementToPerformAction] = [...parameters.props]
const propertyOpenedSubtree = { open: true, items: [{ a: 1 }], siblings: [], hasSubtree: true }
const propertyOpenedSubtree = {
open: true,
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
expanded: true,
items: [{ a: 1 }],
siblings: [],
hasSubtree: true,
}
const expectedKeyNumberVertical = parameters.behavior(propertyOpenedSubtree).keyActions[
elementToPerformAction
][action].keyCombinations[0].keyCode
Expand All @@ -610,7 +685,7 @@ definitions.push({
regexp: /Triggers '(\w+)' action with '(\w+)' on '([\w-]+)', when has a closed subtree\./g,
testMethod: (parameters: TestMethod) => {
const [action, key, elementToPerformAction] = [...parameters.props]
const propertyClosedSubtree = { open: false, hasSubtree: false }
const propertyClosedSubtree = { open: false, expanded: false, hasSubtree: false }
const expectedKeyNumberVertical = parameters.behavior(propertyClosedSubtree).keyActions[
elementToPerformAction
][action].keyCombinations[0].keyCode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ describe('TreeItemBehavior', () => {
})

test(`is added with 'false' value to an item that is expandable but not open`, () => {
const expectedResult = treeItemBehavior({ hasSubtree: true, open: false })
const expectedResult = treeItemBehavior({ hasSubtree: true, expanded: false })
expect(expectedResult.attributes.root['aria-expanded']).toEqual(false)
})

test(`is added with 'false' value to an item that is expandable and open`, () => {
const expectedResult = treeItemBehavior({ hasSubtree: true, open: true })
const expectedResult = treeItemBehavior({ hasSubtree: true, expanded: true })
expect(expectedResult.attributes.root['aria-expanded']).toEqual(true)
})
})
Expand Down
43 changes: 36 additions & 7 deletions packages/react/src/components/Tree/Tree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
ShorthandValue,
} from '../../types'
import { hasSubtree, removeItemAtIndex } from './lib'
import { TreeTitleProps } from './TreeTitle'
import TreeTitle, { TreeTitleProps } from './TreeTitle'
import { ReactAccessibilityBehavior } from '../../lib/accessibility/reactTypes'

export interface TreeSlotClassNames {
Expand All @@ -37,13 +37,13 @@ export interface TreeProps extends UIComponentProps, ChildrenComponentProps {
/** Accessibility behavior if overridden by the user. */
accessibility?: Accessibility

/** Ids of opened items. */
/** Ids of expanded items. */
activeItemIds?: string[]

/** Initial activeItemIds value. */
defaultActiveItemIds?: string[]

/** Only allow one subtree to be open at a time. */
/** Only allow one subtree to be expanded at a time. */
exclusive?: boolean

/** Shorthand array of props for Tree. */
Expand Down Expand Up @@ -112,6 +112,9 @@ class Tree extends AutoControlledComponent<WithAsProp<TreeProps>, TreeState> {

static autoControlledProps = ['activeItemIds']

static Item = TreeItem
static Title = TreeTitle

// memoize this function if performance issue occurs.
static getItemsForRender = (itemsFromProps: ShorthandCollection<TreeItemProps>) => {
const itemsForRenderGenerator = (
Expand Down Expand Up @@ -146,9 +149,35 @@ class Tree extends AutoControlledComponent<WithAsProp<TreeProps>, TreeState> {
}

static getAutoControlledStateFromProps(nextProps: TreeProps, prevState: TreeState) {
const itemsForRender = Tree.getItemsForRender(nextProps.items)
const { items } = nextProps
const itemsForRender = Tree.getItemsForRender(items)
let { activeItemIds } = nextProps

if (!activeItemIds && items) {
activeItemIds = prevState.activeItemIds

const expandedItemsGenerator = (items, acc = activeItemIds) =>
_.reduce(
items,
(acc, item) => {
if (item['expanded'] && acc.indexOf(item['id']) === -1) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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 ?

acc.push(item['id'])
}

if (item['items']) {
return expandedItemsGenerator(item['items'], acc)
}

return acc
},
acc,
)

expandedItemsGenerator(items)
}

return {
activeItemIds,
itemsForRender,
}
}
Expand Down Expand Up @@ -273,14 +302,14 @@ class Tree extends AutoControlledComponent<WithAsProp<TreeProps>, TreeState> {
const itemForRender = itemsForRender[item['id']]
const { elementRef, ...restItemForRender } = itemForRender
const isSubtree = hasSubtree(item)
const isSubtreeOpen = isSubtree && this.isActiveItem(item['id'])
const isSubtreeExpanded = isSubtree && this.isActiveItem(item['id'])
const renderedItem = TreeItem.create(item, {
defaultProps: () => ({
accessibility: accessibility.childBehaviors
? accessibility.childBehaviors.item
: undefined,
className: Tree.slotClassNames.item,
open: isSubtreeOpen,
expanded: isSubtreeExpanded,
renderItemTitle,
key: item['id'],
contentRef: elementRef,
Expand All @@ -292,7 +321,7 @@ class Tree extends AutoControlledComponent<WithAsProp<TreeProps>, TreeState> {
return [
...renderedItems,
renderedItem,
...(isSubtreeOpen ? renderItems(item['items']) : ([] as any)),
...(isSubtreeExpanded ? renderItems(item['items']) : ([] as any)),
]
},
[],
Expand Down
Loading