Tree bug fixes introduced by keyboard navigation #1379
Conversation
- Tree is not rendered on props changes - Show proper aria-label value when label is a node
| @@ -54,16 +54,6 @@ class Tree extends React.Component { | |||
| }); | |||
| } | |||
|
|
|||
| shouldComponentUpdate (nextProps, nextState) { | |||
There was a problem hiding this comment.
It turns out that it's simpler to avoid the react lifecycle completely by moving treeHasFocus out of state.
There was a problem hiding this comment.
What I'm seeing here is something like treeHasFocusDueToClick? Would that be more accurate? I'll go ahead and push that variable name if you agree.
There was a problem hiding this comment.
Yes that's right, go ahead. Thanks!
onClick -> onSelect onExpandClick -> onExpand
| if (isFunction(props.onClick)) { | ||
| props.onClick(event, { | ||
| if (isFunction(props.onSelect)) { | ||
| props.onSelect(event, { |
There was a problem hiding this comment.
@garygong Removing this line makes the expand work.
| @@ -60,7 +61,7 @@ const DemoTree = createReactClass({ | |||
| ? sampleNodes[this.props.exampleNodesIndex] | |||
| : sampleNodes.sampleNodesDefault; | |||
| return { | |||
| nodes: initalNodes, | |||
| nodes: cloneDeep(initalNodes), | |||
There was a problem hiding this comment.
I was seeing that nodes values were carrying over test-to-test. We should start fresh every time.
components/tree/index.jsx
Outdated
| @@ -29,15 +31,27 @@ import { TREE } from '../../utilities/constants'; | |||
| class Tree extends React.Component { | |||
| constructor (props) { | |||
| super(props); | |||
| const flattenedNodes = this.flattenTree({ | |||
There was a problem hiding this comment.
Now, we find the first selected node and initialize it properly so that can be tabbed to. If no node is selected, it will be selected upon first focus.
components/tree/private/branch.jsx
Outdated
| @@ -173,7 +172,7 @@ const handleKeyDown = (event, props) => { | |||
| }; | |||
|
|
|||
| const handleFocus = (event, props) => { | |||
| if (!props.focusedNodeIndex) { | |||
| if (!props.focusedNodeIndex && event.target === event.currentTarget) { | |||
There was a problem hiding this comment.
This selects the node upon first focus, but only do it if focusing the <li>. When focusing the button, we allow it to go through so that the branch expand handler works.
This allows manual testing of setting tabIndex/focus not on the first item.
interactivellama
force-pushed
the
tree-fixes
branch
from
068797a to
fe4aa23
Compare
Move flattenTree function out and make other methods fat arrows
This prevents data reference leaking between stories
interactivellama
changed the title
This fixes two Tree bugs:
Pull Request Review checklist (do not remove)
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.