- Notifications
You must be signed in to change notification settings - Fork921
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 fortemplateFiles andpreviousTemplateVersion 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 aroundundefined) that I'm guessing that the compiler isn't complaining about right now, because we don't have it set strictly enough.
Here's aTypeScript 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