[terra-folder-tree] Add isSelectable prop #2021
Conversation
| /** | ||
| * Indicates whether the item is selected. Because this component has the appearance of a radio button group, only one item should be selected at a time. | ||
| */ | ||
| isSelected: PropTypes.bool, | ||
| /** | ||
| * The callback function for a click event. | ||
| */ | ||
| onClick: PropTypes.func, | ||
| onSelect: PropTypes.func, |
There was a problem hiding this comment.
Renamed this prop from onClick to onSelect to better reflect its actual intended usage. If there are concerns about passivity this can be reverted.
There was a problem hiding this comment.
I think the passivity concern would be if MPages is already using this callback. If they have coded to use onClick, this minor version update would break them, correct?
There was a problem hiding this comment.
Yes, this will break consumers who are already using onClick.
There was a problem hiding this comment.
The component is still in alpha, so I think we can get away with making a non-passive change. The prop rename makes sense to me. I would just add a "breaking change" entry in the CHANGELOG.
There was a problem hiding this comment.
Good point, added the breaking change entry here: 7555293
| {/* eslint-disable-next-line react/forbid-dom-props */} | ||
| <span style={{ paddingLeft: `${level}rem` }}> | ||
| <span style={{ paddingLeft: `${level * 14}px` }}> |
There was a problem hiding this comment.
Why are we multiplying the level by 14? Also, is there a reason to remove rem? If you are trying to convert rem to px, this is not the correct way to convert.
There was a problem hiding this comment.
14px is the correct value as specified by UX, the original value was incorrect.
| className={cx('radio')} | ||
| /> | ||
| ) : ( | ||
| <div className={cx('radio-container')} /> |
There was a problem hiding this comment.
A div is a block element. Do you want an actual div? Can we not just add the necessary padding or margin to the sibling element?
| .radio-container { | ||
| height: 14px; | ||
| margin: 17px; | ||
| width: 14px; | ||
| } |
There was a problem hiding this comment.
It looks like you created a new class that duplicates the styles of the radio class. Maybe you could give a meaningful name so that you can have a single class.
| @@ -169,23 +196,16 @@ const FolderTreeItem = ({ | |||
| className={itemClassNames} | |||
| role="treeitem" | |||
| aria-expanded={isFolder ? isExpanded : null} | |||
| aria-selected={isSelected} | |||
| onClick={isFolder ? handleToggle : onClick} | |||
| aria-selected={isSelectable ? isSelected : null} | |||
There was a problem hiding this comment.
You should be able to just use the isSelected value, right?
There was a problem hiding this comment.
According to the mdn docs, the states for aria-selected are true (the element is selectable and selected), false (the element is selectable and unselected), and undefined (the element is not selectable). I'd like to avoid setting aria-selected={false} on elements that are not selectable (although it does seem to behave correctly in Voiceover at least) since it implies that the element is selectable.
packages/terra-folder-tree/src/subcomponents/FolderTreeItem.jsx
Outdated
Show resolved
Hide resolved
chrismichalewicz
added
⭐ Accessibility Reviewed
and removed
Accessibility Review Ready
labels
|
@sycombs I have reviewed for a11y and it sounds good, great work! I have updated the a11y label. Tested on a PC with Microsoft Edge Version 121.0.2277.128, JAWS 2022 version 2022.2202.38
|
Summary
This PR adds an
isSelectableprop to FolderTreeItem so that consumers can define whether or not an item can be selected.Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-10086
Thank you for contributing to Terra.
@cerner/terra