feat: Add new props for search, keyboard navigation, and scrolling #589
Conversation
gandhis1
changed the title
gandhis1
force-pushed
the
search_state
branch
3 times, most recently
from
49430a8
to
29d1fd7
Compare
The new prop searchTerm can be used to set the initial search term or fully control the search semantics for a higher-order component. The new prop onSearchChange allows specification of a callback for when the search term changes or the search mode is activated or deactivated. The new prop disableKeyboardNavigation allows disabling all actions associated with a key down event in the search input box. The new prop pageSize can be used to control the size of the scroll view before scrolling to near the bottom is required to show more nodes.
| } | ||
|
|
||
| static defaultProps = { | ||
| onAction: () => {}, | ||
| onFocus: () => {}, | ||
| onBlur: () => {}, | ||
| onChange: () => {}, | ||
| onSearchChange: (_) => {}, |
There was a problem hiding this comment.
There is an incompatibility between the prettier pre-commit hook and the codeclimate checks on GitHub with regard to this line. One tool insists on parentheses being present, the other insists it it on being absent. Is there any way we can use the same linter on both so that one tool does not conflict with the other like this?
There was a problem hiding this comment.
Hmm, not sure. Will have to check with CC docs to see if they support a lint rc.
There was a problem hiding this comment.
We can ignore the warning manually in CC dashboard as a last resort
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions. |
|
Bump |
| /** dsiableKeyboardNavigation will disable keyboard navigation of the tree */ | ||
| dsiableKeyboardNavigation?: boolean |
There was a problem hiding this comment.
| /** dsiableKeyboardNavigation will disable keyboard navigation of the tree */ | |
| dsiableKeyboardNavigation?: boolean | |
| /** disableKeyboardNavigation will disable keyboard navigation of the tree */ | |
| disableKeyboardNavigation?: boolean |
| mode: PropTypes.oneOf(['multiSelect', 'simpleSelect', 'radioSelect', 'hierarchical']), | ||
| pageSize: PropTypes.number, |
There was a problem hiding this comment.
@gandhis1 I did think about it during the infinite scroll implementation but ultimately decided against it. Reasons being
- an arbitrary pageSize doesn't help with the overall intent of optimization - e.g, setting a lower number is actually counter productive and setting a higher number will slow down the rendering. This gets exacerbated if you have multiple dropdowns on the page. Furthermore, unless you've a really big screen, you can't see more than 100 anyway so why hold it in DOM?
| @@ -50,18 +53,21 @@ class DropdownTreeSelect extends Component { | |||
| inlineSearchInput: PropTypes.bool, | |||
| tabIndex: PropTypes.number, | |||
| disablePoppingOnBackspace: PropTypes.bool, | |||
| disableKeyboardNavigation: PropTypes.bool, | |||
There was a problem hiding this comment.
What's the use case for this? At a glance it seems like an a11y blocker but people don't have to use the keyboard if they don't want to so curios as to why this is needed.
There was a problem hiding this comment.
From my PR description above:
The new prop disableKeyboardNavigation allows disabling all actions associated with a key down event in the search input box. This is important if you need an input box to behave normally. For instance, when I press SHIFT+HOME, it should select the entire line, so I can press BACKSPACE to clear it.
I can't use arrow keys either on the search box right now.
There was a problem hiding this comment.
an input box to behave normally
I hear you but it's not really a regular input box - hence the special treatment.
when I press SHIFT+HOME, it should select the entire line, so I can press BACKSPACE to clear it.
Can this handled by adding additional functionality to the keyboard navigation logic? So that we don't have to choose between having it or not at all?
|
Hello! Curious on the status of this pr, this change would help out my use case of not clearing the search term on selection. Thanks! |
What does it do?
searchTermcan be used to set the initial search term or fully control the search semantics for a higher-order component.onSearchChangeallows specification of a callback for when the search term changes or the search mode is activated or deactivated.disableKeyboardNavigationallows disabling all actions associated with a key down event in the search input box. This is important if you need an input box to behave normally. For instance, when I press SHIFT+HOME, it should select the entire line, so I can press BACKSPACE to clear it.pageSizecan be used to control the size of the scroll view before scrolling to near the bottom is required to show more nodes.Fixes # (issue)
Fixes #581
Requested #463
Fixes #576
Type of change
Please delete options that are not relevant.
Checklist: