-
Notifications
You must be signed in to change notification settings - Fork 887
feat(site): show version files diff based on active version #11686
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
Conversation
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 all looks really good! Just had one possible suggestion, but I don't know enough about the business to logic to say if it's a good idea
@@ -236,6 +237,38 @@ export const resources = (versionId: string) => { | |||
}; | |||
}; | |||
|
|||
export const templateFiles = (fileId: string) => { | |||
return { | |||
queryKey: ["templateFiles", fileId], |
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.
Do you think it makes sense to update the query keys for templateFiles
and previousTemplateVersion
to start with something like "template"
? Feels like it'd be a good way to make sure we have an easy way to invalidate any template queries down the line
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.
If we had the template ID or name I would say yes, but since they are "isolated" I'm ok to keep the current names until we see value in changing them.
tab, | ||
}) => { | ||
const filenames = Object.keys(currentFiles); | ||
const selectedFilename = filenames[Number(tab.value)]; | ||
const currentFile = currentFiles[selectedFilename]; | ||
const previousFile = previousFiles && previousFiles[selectedFilename]; | ||
const previousFile = baseFiles && baseFiles[selectedFilename]; |
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.
Not a call out to your code, but there's some weird behavior here (based around undefined
) that I'm guessing that the compiler isn't complaining about right now, because we don't have it set strictly enough.
Here's a TypeScript snippet with the compiler tightened up. I'm going to be bringing this up at the Frontend Variety later today
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.
Yes, I think we should set this rule as true: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-1.html#checked-indexed-accesses---nouncheckedindexedaccess
Close #11295