feat(TreeView): allow selection without expansion by kmcfaul · Pull Request #7714 · patternfly/patternfly-react

kmcfaul · 2022-07-19T15:04:13Z

What: Closes #7378

Adds hasSelectableNodes prop that separates expansion & selection behaviors - clicking the row will select it while clicking the toggle button will trigger the expansion. This flag will also allow parent nodes to be selected (in addition to leaf nodes) and apply the current style modifier to them.

kmcfaul · 2022-07-19T15:06:28Z

@mcoker One thing I noticed on the core PR was that the pf-c-tree-view__node was still a button (and the toggle was in a span) in the With selectable, expandable nodes example, which I think was the example showcasing this behavioral change, but in your PR description you mentioned that the node should now be a div with the toggle changing to a button. I went with the div > button structure right now but let me know if I misunderstood and should change something.

patternfly-build · 2022-07-19T15:15:14Z

Preview: https://patternfly-react-pr-7714.surge.sh

A11y report: https://patternfly-react-pr-7714-a11y.surge.sh

mcoker · 2022-07-19T23:35:57Z

@kmcfaul sorry for the confusion, we made another update after I opened the PR and I didn't update the description. What's in the PR URL, or maybe more accurately the core workspace. Can you go from that? Or would you prefer I go through the changes and update the PR description?

thatblindgeye

I'm noticing issues when trying to navigate/interact with the example via VO or keyboard. Essentially I can select a tree item fine, but I can't find a way to toggle the expand state in anyway. See the following video (using VO):

Tree.view.selection.and.expandable.VO.issue.mov

The "with checkboxes" example works a bit better in terms of VO/keyboard being able to expand/collapse a tree item or check/uncheck the checkbox without one affecting the other. I made some comments below about what I tried doing locally to try and resolve this.

thatblindgeye · 2022-07-20T17:35:01Z

@@ -94,14 +97,23 @@ const TreeViewListItemBase: React.FunctionComponent<TreeViewListItemProps> = ({
  }, [isExpanded, defaultExpanded]);

  const Component = hasCheck ? 'div' : 'button';


I was able to replicate the behavior from the "with checkboxes" example by updating this to hasCheck || hasSelectableNodes ? 'div' : 'button' (more info in another comment below)

mcoker

tl;dr - Looking at this PR, looks like we need to:

Make .pf-c-tree-v-ew__node-container a span
Make .pf-c-tree-view__node-toggle a span in expandable, non-checkbox nodes
Looks like all nodes of a selectable tree view are getting .pf-m-selectable - only the expandable node needs it.
Make checkbox nodes a label, and change the node-text element inside to a span
Add .pf-m-selectable to the expandable checkbox nodes

I also found a few bugs and put up a core PR - patternfly/patternfly#4990

Reviewing the core PR, the changes are:

A tree view node has .pf-m-selectable when it's expandabale/selectable (including checkbox nodes)
These elements changed to a <span> for HTML validity reasons, not a big deal:
- .pf-c-tree-view__node-container
- .pf-c-tree-view__node-content
- .pf-c-tree-view__node-toggle when it isn't a button
Checkbox .pf-c-tree-view__node is now a <label>
- .pf-c-tree-view__node-text is a <span> since it was a <label>
- HTML label attrs moved from node-text to node
Updated hover states on the toggle icon so that it changes color when you're hovering the thing that will toggle expansion. ie, the node if non-selectable, the toggle if selectable.

With the intention that basically

A default tree view node is a button, the toggle is a span. The whole node toggles expansion, the toggle icon is just for show and changes colors when you hover/focus on the node.
A checkbox node is a label, and the toggle inside is a button. Clicking on the toggle will toggle expansion, clicking anywhere else will trigger the checkbox to be checked/selected.
A non-checkbox selectable node is a button and the toggle is a span (since you can't nest a button in a button). Clicking the toggle toggles the node expansion, clicking anywhere else selects the node.

mmenestr · 2022-07-21T21:38:46Z

This looks good overall, my only question is whether we should allow someone to select a full expandable node in a tree view - or whether they should only be allowed to select a child node with no expansion?

kmcfaul · 2022-07-27T13:51:55Z

@mmenestr That is the default behavior if you are referring to the blue bold styling that is applied to the selected node. I adjusted it to apply to a parent node because the new flag allows a parent node to be selected directly now, but I can change it back if we don't want that visually. LMK

thatblindgeye · 2022-07-27T16:48:52Z

I opened #7759 as a followup for the keyboard nav issues

mmenestr · 2022-07-27T20:22:16Z

Ok, no no if we're allowing a parent node to be selected then it should have that styling! Looks good!

mcoker

LGTM other than the keyboard issue @thatblindgeye noted and one small nit on the for and id attrs on nodes.

I did rework the aria-labels on the nodes and toggles in this PR if we want to consider that - patternfly/patternfly#4991

mcoker

LGTM! I'd like to get patternfly/patternfly#4991 merged and pulled in so we can make sure it fixes the toggle button's icon color when that node is selected.

thatblindgeye

Just a few comments below, let me know what you think, but otherwise this looks good!

thatblindgeye · 2022-07-29T12:58:20Z

+  /** Flag indicating that tree nodes should be independently selectable, even when having children */
+  hasSelectableNodes?: boolean;


Nitpick that I wouldn't block over: isSelectable could be an alternative prop name, as it would also align with our Card and Table selectable variants that have a similar prop.

Yeah the prop name is something I wasn't too sure on. I thought about isSelectable but wasn't sure if it would be clear that it would make the nodes selectable opposed to the whole tree. I'm open to either prop name if anyone feels strongly one way or the other.

Ah that's a fair point. I might think that having selectable items would make the tree view selectable in nature, but I do see how it isn't exactly clear. How do you feel about the idea of TreeView with prop hasSelectableNodes, and the TreeViewListItem with prop isSelectable?

I like that, will update

mcoker · 2022-07-29T17:38:52Z

@kmcfaul patternfly/patternfly#4991 has been merged and is in core v4.206.1

tlabaj

lgtm

feat(TreeView): allow selection without expansion

0801291

update structure, remove extra button from example

e5edf32

nicolethoen added ux review A11y labels Jul 20, 2022

nicolethoen requested review from mcoker, mmenestr and thatblindgeye July 20, 2022 16:02

thatblindgeye reviewed Jul 20, 2022

View reviewed changes

mcoker requested changes Jul 21, 2022

View reviewed changes

further html updates

11d4d58

kmcfaul added 6 commits July 25, 2022 11:45

update structure

9ae280a

add id to toggle button

b6c4d2c

apply id only to selectable nodes and update test

dbbb826

fix duplicate ids

42bbd4f

match checkbox aria

bae969f

update snap

6af83a2

thatblindgeye mentioned this pull request Jul 27, 2022

Bug - Tree view: separate selection/expansion example cannot operate via keyboard #7759

Closed

mmenestr approved these changes Jul 27, 2022

View reviewed changes

mcoker requested changes Jul 27, 2022

View reviewed changes

Comment thread packages/react-core/src/components/TreeView/TreeViewListItem.tsx Outdated

remove htmlfor from selectable nodes

e5773f9

mcoker approved these changes Jul 29, 2022

View reviewed changes

thatblindgeye requested changes Jul 29, 2022

View reviewed changes

kmcfaul added 2 commits July 29, 2022 11:38

pr feedback

f05e46d

rename node prop

32f881f

thatblindgeye approved these changes Jul 29, 2022

View reviewed changes

tlabaj approved these changes Jul 29, 2022

View reviewed changes

tlabaj merged commit fed4ff6 into patternfly:main Jul 29, 2022

mcoker mentioned this pull request Aug 2, 2022

Bug - Tree view - selected state doesn't persist if parent node is collapsed #7785

Closed

		@@ -94,14 +97,23 @@ const TreeViewListItemBase: React.FunctionComponent<TreeViewListItemProps> = ({
		}, [isExpanded, defaultExpanded]);

		const Component = hasCheck ? 'div' : 'button';

		/** Flag indicating that tree nodes should be independently selectable, even when having children */
		hasSelectableNodes?: boolean;

Conversation

kmcfaul commented Jul 19, 2022

Uh oh!

kmcfaul commented Jul 19, 2022

Uh oh!

patternfly-build commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Jul 19, 2022

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mmenestr commented Jul 21, 2022

Uh oh!

kmcfaul commented Jul 27, 2022

Uh oh!

thatblindgeye commented Jul 27, 2022

Uh oh!

mmenestr commented Jul 27, 2022

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thatblindgeye Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

kmcfaul Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

kmcfaul Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

mcoker commented Jul 29, 2022

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

patternfly-build commented Jul 19, 2022 •

edited

Loading