fix: merged compose deploy to kube page in UI #4827
Conversation
When used on a compose app, the Deploy to Kube form incorrectly shows the compose details at the bottom of the page. You can also see it in the breadcrumb that says 'Container > Compose Details' when it should say 'Container > Deploy to Kubernetes'. The problem is the route path of these two pages can overlap: - /compose/deploy-to-kube/:composeGroupName/:engineId/* - /compose/:name/:engineId/* i.e. deploy to kube path also resolves to compose details page with name 'deploy-to-kube' and engineId of the compose group name. Simple fix to just rename the path so they don't overlap. Signed-off-by: Tim deBoer <git@tdeboer.ca>
packages/renderer/src/App.svelte
Outdated
| @@ -132,7 +132,7 @@ window.events?.receive('display-troubleshooting', () => { | |||
| type="container" /> | |||
| </Route> | |||
| <!-- Same DeployPodToKube route, but instead we pass in the compose group name, then redirect to DeployPodToKube --> | |||
| <Route path="/compose/deploy-to-kube/:composeGroupName/:engineId/*" breadcrumb="Deploy to Kubernetes" let:meta> | |||
| <Route path="/compose-deploy-to-kube/:composeGroupName/:engineId/*" breadcrumb="Deploy to Kubernetes" let:meta> | |||
There was a problem hiding this comment.
I would keep the /compose namespace to all the compose routing and use other differentiators instead of having separate routes at the beginning
-> change routing scheme after /compose rather than at the root level
if we have common prefix we can route or use components for the routing (I think @feloy wanted to introduce routing scheme/helpers at some point) but if we break at the root level, it'll be harder to introduce Routing components per scope
There was a problem hiding this comment.
Ok, second commit reverts the deploy-to-kube path and changes the details path instead. I just don't love this option because it makes the compose details different from image/volumes/pods/etc details instead, but we have to change one of them.
There was a problem hiding this comment.
we could change the route for image/volumes/pods/etc to be consistent 👍
There was a problem hiding this comment.
I liked the approach of @feloy to have helpers for the routes so we don't duplicate everywhere the route path
There was a problem hiding this comment.
I found the related PR #4371
There was a problem hiding this comment.
I finally reverted it in this PR, as I didn't have enough perspective at this time. But it is visible in the first commit of the PR
As suggested in the PR this changes the compose details path instead. Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
is it missing details there: ? |
|
ping about the last comment |
Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
Sorry, was distracted by other PRs. Yes, I missed that reference. Added in new commit, and I did a scan again to confirm there are no further references. |
What does this PR do?
When used on a compose app, the Deploy to Kube form incorrectly shows the compose details at the bottom of the page. You can also see it in the breadcrumb that says 'Container > Compose Details' when it should say 'Container > Deploy to Kubernetes'.
The problem is the route path of these two pages can overlap:
Simple fix to just rename the path so they don't overlap.
Screenshot/screencast of this PR
Same as before, but with correct breadcrumb and no details page at the bottom!
What issues does this PR fix or reference?
Fixes #4723.
How to test this PR?
Create a compose app and use the Deploy to Kubernetes action on the Containers page.