Skip to content

feat: Add diff and Dockerfile support for template version page#5339

Merged
BrunoQuaresma merged 17 commits into
mainfrom
bq/add-diff
Dec 8, 2022
Merged

feat: Add diff and Dockerfile support for template version page#5339
BrunoQuaresma merged 17 commits into
mainfrom
bq/add-diff

Conversation

@BrunoQuaresma
Copy link
Copy Markdown
Contributor

@BrunoQuaresma BrunoQuaresma commented Dec 7, 2022

Demo:

Screen.Recording.2022-12-06.at.18.49.24.mov

@BrunoQuaresma BrunoQuaresma requested a review from a team December 7, 2022 16:49
@BrunoQuaresma BrunoQuaresma self-assigned this Dec 7, 2022
@BrunoQuaresma BrunoQuaresma marked this pull request as ready for review December 7, 2022 16:49
@BrunoQuaresma BrunoQuaresma requested a review from a team as a code owner December 7, 2022 16:49
@BrunoQuaresma BrunoQuaresma requested review from Kira-Pilot and removed request for a team December 7, 2022 16:49
@BrunoQuaresma BrunoQuaresma enabled auto-merge (squash) December 7, 2022 18:03
Copy link
Copy Markdown
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few comments but other than that, BE looks good. 👍🏻

And wrt frontend, I think this solution is a great start. It'd be pretty cool if we opened up an instance of code-server or something that showed the files/diff, but IDK if that'd make our bundle size way too huge. 😂

Comment thread coderd/database/databasefake/databasefake.go
Comment thread coderd/database/queries/templateversions.sql
Bruno Quaresma and others added 2 commits December 8, 2022 11:23
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
@BrunoQuaresma
Copy link
Copy Markdown
Contributor Author

And wrt frontend, I think this solution is a great start. It'd be pretty cool if we opened up an instance of code-server or something that showed the files/diff, but IDK if that'd make our bundle size way too huge. 😂

Yeah, but we are basically using the editor that is used by VS Code. https://microsoft.github.io/monaco-editor/

Comment thread site/src/api/api.ts

export type GetPreviousTemplateVersionByNameResponse =
| TypesGen.TemplateVersion
| undefined
Copy link
Copy Markdown
Member

@Kira-Pilot Kira-Pilot Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make a type for this on the BE? Maybe in codersdk/templateversions.go? Nvmd, I see what's happening

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!


if (coderTheme.isLoading) {
return null
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a loading state?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, it is VERY fast. It is only a few milliseconds to monaco be loaded. From what I was able to inspect there is no networking involved so the "loading time" is "static"

},
})

export const useCoderTheme = (): { isLoading: boolean; name: string } => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a comment here explaining what this hook/what its scope is might be helpful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

Copy link
Copy Markdown
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done; very clean!

@BrunoQuaresma BrunoQuaresma merged commit ce76d9d into main Dec 8, 2022
@BrunoQuaresma BrunoQuaresma deleted the bq/add-diff branch December 8, 2022 15:24
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants