-
Notifications
You must be signed in to change notification settings - Fork 55
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
refactor(template): utilize new GET /v1/templates/{id} endpoint for template and deployment detail #506
base: main
Are you sure you want to change the base?
Conversation
d3c6b21
to
6f931cb
Compare
…emplate and deployment detail refs #477
6f931cb
to
bc893c6
Compare
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.
Cool! Were you planning on changing the template list endpoint in another pr?
|
||
export default DeploymentDetailPage; | ||
export const getServerSideProps = getValidatedServerSideProps(contextSchema, async ({ params }) => { | ||
const remoteDeployTemplate = await services.template.findById(CI_CD_TEMPLATE_ID); |
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 don't think we necessarily need to have loaded on the server. Especially since it's for a specific feature (Git repo deploy) that not that many deployments have currently.
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.
looking into the logic in the components it seems like this template is still needed there. Perhaps this could be changed ofc.
But I was thinking of how often that template is changed? perhaps let's move it to static props and call it a day?
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.
Previously it was loaded through a hook,, but now it'll wait for the template to load to render the page. It should be fast enough that it doesn't affect the ux, but generally I'd put it as a fetch hook in the component.
This PR doesn't complete the issue, yes. So more is to come. May be I'll switch to some other outstanding things though. I was just pushing what's done. It's ready to merge though |
Ok sounds good! |
refs #477