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 😞
| onClick: () => void; | ||
| label: string; | ||
| className?: string; | ||
| }; |
There was a problem hiding this comment.
If SearchButton becomes a very generic component:
- Does it always need a label as a prop?
- If we decide that it may exist without a label, should it in that case be able to have an aria-label for screen readers?
- Could you add it to Storybook?
| const { match, genomeIdForUrl } = props as { | ||
| match: GeneSearchMatch; | ||
| genomeIdForUrl: string; | ||
| }; |
There was a problem hiding this comment.
The type of props should probably be defined when the function is defined. It seems odd to be redefining the props type here.
| getQueries: (state) => state.queries, | ||
| getGeneQuery: (state) => state.queries.gene, | ||
| getVariantQuery: (state) => state.queries.variant | ||
| } |
There was a problem hiding this comment.
I don't think this selectors block is doing any work here. You defined the selectors in the speciesSelectorFeatureSearchSelectors file, and you are not exporting these selectors from this slice file.
There was a problem hiding this comment.
Remind me: why are we keeping the state in a redux store? Why isn't it local to the component (or to the input DOM element)?
There was a problem hiding this comment.
I think I modified existing store. Helps to retain query state of both search modes during route change and reload.
wip