[Package][Genetics] Fix search for Genetics #356
+35
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
|
Thank you @riyavsinha 👍 @chinmehta will be taking a look at the code during the week |
chinmehta
reviewed
May 18, 2024
| @@ -13,15 +13,19 @@ function useListOption() { | |||
| if (newOption.entity === "search") { | |||
| history.push(`/search?q=${newOption.name}&page=1`); | |||
| } else if (newOption.entity === "study") { | |||
| history.push(`/${newOption.entity}/${newOption.studyId}`); | |||
| if (newOption.studyId) { | |||
There was a problem hiding this comment.
at this point we should use switch instead of if conditional statements
| if (isArray(value)) { | ||
| value.map(i => | ||
| typesArray.push({ | ||
| type: key === "topHit" ? "topHit" : key, | ||
| entity: key, | ||
| type: key === "topHit" ? "topHit" : i.__typename.toLowerCase(), |
There was a problem hiding this comment.
Apollo primarily provides __typename for its own functions like caching, we should try to use the response that is present in the schema.
| </Box> | ||
| ); | ||
| const SearchSuggestionEl = | ||
| searchSuggestions.length > 0 ? ( |
There was a problem hiding this comment.
It is good to have a condition, although it is preferred that we modify and pass EXAMPLES from Genetics > HomePage.tsx so that we don't see empty search container.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current state of search for genetics:
Description
First issue was that there were no default
searchSuggestionsfor the genetics app provided in theSearchContext, so mapping over the undefined object caused errors. This was fixed by making the defaultsearchSuggestionsan empty array. Also, I only now render the suggestions element + header whensearchSuggestionsis non-empty.The second issue is that it seems the routing logic was only written/updated for the Platform and not Genetics? The
searchUtilsdata formatting for Genetics turns the entity types intogenes,variantsorstudies, which cause bad urls that do not work. Using__typeNameto lowercase does work for now. This does not affect the Platform because the Platform search results use thevalue.hitsarray style. Additionally, theuseListOptionURL construction adds on/associationsfor gene and variant type entities, and requires astudyIdfor studies which is not present in the Genetics applicationstudyentities it seems. I have modified that code to account for that for now.However, I believe a better longer term solution would be for each application to pass in an
entityToUrlMapperfunction to theuseListOptionhook, since that hook is supposed to be application-agnostic.Issue: # (link)
Deploy preview: (link)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
New state for genetics:
Checklist: