Improvements for Picker component #336
Conversation
| if (!disabledSearch) { | ||
| document.removeEventListener('keydown', onKeyDown); | ||
| } | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
I think 'disabledSearch' and 'onKeyDown' should be added to the dependency table. Linter didn't complain about that?
| size?: TriggerSize; | ||
| placeholder?: string; | ||
| isRequired?: boolean; | ||
| noSearchResultText?: string; | ||
| disableSearch?: boolean; |
There was a problem hiding this comment.
Aren't we following any naming convention for booleans?
Verbs should be restricted for functions/methods, while boolean should have an interrogative form, eg. isSearchDisabled. The only exception is HTML syntax, eg. disabled.
Alternatively I'd consider searchDisabled
| @@ -14,6 +14,7 @@ $base-class: 'picker-list'; | |||
| padding: 4px 0; | |||
| position: absolute; | |||
| width: 341px; | |||
| z-index: 10000; | |||
There was a problem hiding this comment.
Is there any rule on how we use z-indexes? Is there any stacking context?
| @@ -14,6 +14,7 @@ const baseClass = 'picker-trigger'; | |||
| export type TriggerSize = 'compact' | 'medium' | 'large'; | |||
|
|
|||
| export interface ITriggerProps { | |||
| disabledSearch: boolean; | |||
There was a problem hiding this comment.
I'd consider alternative naming as in case of Picker
There was a problem hiding this comment.
Plus there is a difference -> disableSearch vs disabledSearch
sgraczyk
changed the title
| @@ -14,7 +14,7 @@ const baseClass = 'picker-trigger'; | |||
| export type TriggerSize = 'compact' | 'medium' | 'large'; | |||
|
|
|||
| export interface ITriggerProps { | |||
| disabledSearch: boolean; | |||
| isSearchDisabled: boolean; | |||
There was a problem hiding this comment.
Here we have isSearchDisabled as this is an internal part of Picker component, where our naming convention perfectly fits, the consumer doesn't see it.
There was a problem hiding this comment.
Fine for now, we'll rethink this later
| /** | ||
| * Definition of z-index values to be used across application. | ||
| */ | ||
|
|
||
| $stacking-context-level-dropdown: 10000; |
There was a problem hiding this comment.
It is ok now, however, I was wondering if 1000 would be enough.
Addresses: #337