-
Notifications
You must be signed in to change notification settings - Fork 1.4k
add selectOnFocus prop to allow disabling selection on focus #9415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Would love your review on this one please 🙏🏻 |
|
Thanks for the PR, the team is currently on holiday. We'll review when we are back in the new year. |
snowystinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just using 'Option + Arrow Down/Up' to navigate via keyboard without selecting on focus work for you without needing these changes? Selection is pretty complex, so it'd be better if we didn't need to add another option.
| linkBehavior?: 'action' | 'selection' | 'override', | ||
| /** | ||
| * Whether selection should occur automatically on focus. | ||
| * @default true (when selectionBehavior is 'replace') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be disregarded if selectionBehaviour is not replace?
| it('should not select on focus when selectOnFocus={false} with multiple selection', async () => { | ||
| let onSelectionChange = jest.fn(); | ||
| let {getAllByRole} = render( | ||
| <GridList aria-label="Test" selectionMode="multiple" selectionBehavior="replace" selectOnFocus={false} onSelectionChange={onSelectionChange}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does one select multiple in replace? does shift+arrow down work?
Motivation
We want to allow a user to browse the list using their keyboard without "persisting" the selection.
This is consistent with many implementations out in the wild:
✅ Pull Request Checklist:
📝 Test Instructions:
selectionBehavior="replace"andselectionMode="multiple"selectOnFocus={false}to the GridList