-
Notifications
You must be signed in to change notification settings - Fork 12
Epic Branch Final Step: Merge Epic Branch into Staging #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
Copy routing changes for Navigator from forked repository into main
Copy routing changes for interactions from forked repository into main
Epic Branch Step 2: Merge Routing Navigator
…View Epic Branch Step 3: Merge Routing Interactions View
…Fixed package lock issues
…hanges to fix blank page issue in interactions view
@@ -0,0 +1,113 @@ | |||
import { z, ZodTypeAny } from 'zod' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll write some docstrings for these functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
loadingAmount={loadAmount} | ||
gene={geneticElement} | ||
view={ChromosomeViewerObject} | ||
error={ViewDataError.FAILED_TO_LOAD} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error={ViewDataError.FAILED_TO_LOAD} | |
error={error} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always show the error screen instead of loading, I think the best thing to do here is pass the error value in, however LoadingState expects a ViewDataError so we need to make the changes that I suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way is to just have a separate if condition for when theres an error (this is how the other views do it) which works as well and might be simpler for now (I think eventually we'll want to do it the first way since that way we can have more flexibility for how the query functions can error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm I see why you did it this way, the issue is that the chromosome loader doesn't error on unsuccesful response. If you can update the ChromosomeViewLoader to do that (see below) and just wrap it in a try-catch so that ViewDataError.FAILED_TO_LOAD is thrown whenever it errors out (like the above suggestion) . Ideally LoadingPage should just GenericError types but that's a future problem
const ChromosomeViewLoader = async (
gene: GeneticElement | null,
loadEvent: (progress: number) => void
) => {
const poplar = false
const species = poplar ? 'Populus_trichocarpa' : 'Arabidopsis_thaliana'
const url = `https://bar.utoronto.ca/eplant${
poplar ? '_poplar' : ''
}/cgi-bin/chromosomeinfo.cgi?species=${species}`
const chromosomeViewData: ChromosomeItem[] = await fetch(url)
.then((response) => {
if (!response.ok) {
throw ViewDataError.FAILED_TO_LOAD
}
return response.json()
})
.then((responseObj: ChromosomesResponseObj) => responseObj['chromosomes'])
loadEvent(100)
return {
viewData: chromosomeViewData,
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've making this change in all the views where applicable but I've noticed now as well that when no gene is selected yet all the views(aside from navigator and interactions) will hang on the loading page and not display a valid error. This is because there are no checks for !geneticElement like there is in nav and interactions. I know you mentioned wanting to simplify all of the checks but this has been my solution thus far:
if (!geneticElement) {
return (
<LoadingPage
loadingAmount={loadAmount}
gene={geneticElement}
view={GeneInfoViewMetadata}
error={ViewDataError.UNSUPPORTED_GENE}
></LoadingPage>
)
}
The reason it is working without this in the nav and interactions is because they check for !geneticElement in their loader functions. Not all the views utilize a loader so I've done it the above way for now. Let me know if I shouldn't do this or if you have another suggestion. I'm just going to commit like this and we can go from there
const handleClose = () => { | ||
setMessageOpen(false) | ||
} | ||
const { data, isLoading, isError, error } = useQuery<ChromosomeViewerData>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { data, isLoading, isError, error } = useQuery<ChromosomeViewerData>({ | |
const { data, isLoading, isError, error } = useQuery<ChromosomeViewerData, ViewDataError>({ |
const { data, isLoading, isError, error } = useQuery<ChromosomeViewerData>({ | ||
queryKey: [`chromosome`], | ||
queryFn: async () => { | ||
return ChromosomeViewLoader(geneticElement, setLoadAmount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ChromosomeViewLoader(geneticElement, setLoadAmount) | |
try{ | |
return ChromosomeViewLoader(geneticElement, setLoadAmount) | |
}catch{ | |
throw ViewDataError.FAILED_TO_LOAD | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great thanks for all your hard work @bdls-jamal! Added a small comment regarding fixing the Chromosome view loading/error state and also a nitpick on using reactQuery instead of your own cache for the InteractionsView Gene data.
I couldn't test out the interactions and chromosome view due to cors errors but the Navigator View looks great!
@Yukthiw All of the views are implemented with error={ViewDataError.FAILED_TO_LOAD} among the other things you suggested to change in the Chromosome viewer. I think your suggestion to change is for all views doing it this way, correct? I'm going to change all of them and then if you could review that commit that would be awesome |
…ggested to fix chromosomeviewer but also matching the changes in other views
This branch should have all of the following updates: