Variation search #1229
Variation search #1229
Conversation
| export enum FeatureSearchModeType { | ||
| GENE_SEARCH_MODE = 'Gene', | ||
| VARIANT_SEARCH_MODE = 'Variant' | ||
| } |
There was a problem hiding this comment.
Let's not use enums. Enums are an early typescript feature that doesn't have a direct correspondence in javascript, and that comes with various problems.
| VARIANT_SEARCH_MODE = 'Variant' | ||
| } | ||
|
|
||
| export const FEATURE_SEARCH_MODES: FeatureSearchMode[] = [ |
There was a problem hiding this comment.
If it is a const, why does it live in a types file?
| onClick={() => updateActiveFeatureSearchMode(searchMode)} | ||
| > | ||
| {searchMode.label} | ||
| </TextButton> |
There was a problem hiding this comment.
Disable the button when the search mode is active. You aren't doing this currently, and as a result, the button can still be focused.
| type Props = { | ||
| query: string; | ||
| activeFeatureSearchMode: FeatureSearchMode; | ||
| searchLocation?: string; // Ex: 'sidebar', 'interstitial' |
There was a problem hiding this comment.
To me, this property name is confusing. If its purpose is to convey the context in which the component renders, then perhaps position, or variety, or something of that sort.
Also, why is it optional, and why is it a generic string, rather than just the two possible values?
| searchLocation?: string; // Ex: 'sidebar', 'interstitial' | ||
| onSearchSubmit: (query: string) => void; | ||
| onClear: () => void; | ||
| updateActiveFeatureSearchMode: (mode: FeatureSearchMode) => void; |
There was a problem hiding this comment.
updateActiveFeatureSearchMode -> onSearchModeChange
|
|
||
| type Props = { | ||
| query: string; | ||
| activeFeatureSearchMode: FeatureSearchMode; |
There was a problem hiding this comment.
I wonder if either all 'feature search modes' should come from outside of the component, or the passed in active mode should just be a string. It feels that it should only just be a string.
| setSearchInput(query); | ||
| setShouldDisableSubmit(true); | ||
| } | ||
| }, [query]); |
There was a problem hiding this comment.
You will see things like this all over the codebase (my fault), but React team thinks that this is actually bad. If an effect is used for the sole purpose to update the state, then the state should be updated as part of render instead.
https://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-effect
As a part of some future pull request, we should try to update the linter rules, and address the errors. There will be plenty 😞
2 participants
wip