[terra-folder-tree] Update cursor style for interactable regions #1937
Conversation
| // Stop click propagation to prevent triggering expand/collapse when selecting folders | ||
| event.stopPropagation(); | ||
|
|
||
| onClick(); |
There was a problem hiding this comment.
Since onClick is not required, we either need to check here or when the event handler is defined to prevent an exception.
| onClick={onToggle} | ||
| onKeyDown={onToggle} | ||
| onClick={isFolder ? onToggle : onClick} | ||
| onKeyDown={isFolder ? onToggle : onClick} |
There was a problem hiding this comment.
Isn't there a specific key or keys that should trigger this action? I would not expect this to happen for any key pressed.
There was a problem hiding this comment.
Yes, I took a look at the keyboard Jira (UXPLATFORM-9776) and expand/collapse should be triggered by the right and left arrows and selection should be triggered via the enter key. I went ahead and removed the onKeyDown handler and disabled the eslint error for now, these should be re-enabled as part of the keyboard interaction story.
ccdc63e
| onChange={onClick} | ||
| onClick={handleRadioButtonClick} |
There was a problem hiding this comment.
What would be the difference between onChange and onClick? Do we need both event handlers?
There was a problem hiding this comment.
The onChange handler was originally used instead of onClick to resolve this React failed proptype warning. Unfortunately, stopPropagation will not prevent the row click event if the radio button uses onChange since they are different event types, so I had to use an onClick instead.
An alternative to addressing the React warning would be to set readOnly on the radio, but that seems incorrect to me as well since it's not really a readonly element, so I've left it as onChange for now, but I'm open to other solutions if we don't want to have an extra event handler here.
There was a problem hiding this comment.
I am assuming the row click is getting the event due to bubbling. If that is the case, can't you just check if the event target is the radio button in the handler. If so, you could return instead of executing the logic.
There was a problem hiding this comment.
Yep, and that's much nicer, updated: 460c1f2
There was a problem hiding this comment.
Just to document but out of scope for this story:
There's one example where we get the pointer cursor on an item that can't be selected or expanded, but this is an issue with the example. When consumed properly, all items would be interactable so pointer cursor everywhere is fine.
Summary
This change updates the cursor style for folder tree rows to be a pointer so that there is a visual cue that the regions are clickable. This also updates the selection behavior so that folder rows can only be selected by the radio button and clicking anywhere else on the row will toggle expand/collapse, and non-folder rows can be selected by clicking anywhere in the row.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-9978
Thank you for contributing to Terra.
@cerner/terra