-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix missing Terminal Shortcuts for "shoot" and "cp" scopes. #2339
base: master
Are you sure you want to change the base?
Conversation
@Lappihuan Thank you for your contribution. |
Thank you @Lappihuan for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
…andled anywhere from any parent component Signed-off-by: Oriano de-Stefani <[email protected]>
… the "GARDEN CLUSTER" button and going to terminal shortcuts from there Signed-off-by: Oriano de-Stefani <[email protected]>
@holgerkoser, @grolu You have pull request review open invite, please check |
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.
Thank you for providing a fix. I just have minor suggestions
@@ -129,9 +129,9 @@ export default { | |||
], | |||
setup () { | |||
const { | |||
shootItem, | |||
shootItem = null, |
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.
shootItem = null, | |
shootItem, |
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 would break the context when coming from the button "GARDEN CLUSTER", since then useShootItem
seems to return null
.
GTerminalShortcuts.vue:135 Uncaught (in promise) TypeError: Cannot destructure property 'shootItem' of 'useShootItem(...)' as it is null.
this just tries to handle this gracefully so we still end up with shootItem
as null
.
there might be a better way to solve it in useShootItem
to still return a result object instead of just null
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 seems to come from this default value here:
dashboard/frontend/src/composables/useShootItem.js
Lines 264 to 266 in 97c1357
export function useShootItem () { | |
return inject('shoot-item', null) | |
} |
isShootStatusHibernated, | ||
} = useShootItem() | ||
} = useShootItem() || {} |
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.
} = useShootItem() || {} | |
} = useShootItem() |
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.
same as above
shootItem = null, | ||
} = useShootItem() || {} |
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.
shootItem = null, | |
} = useShootItem() || {} | |
shootItem, | |
} = useShootItem() |
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.
same as above
@grolu, @holgerkoser You have pull request review open invite, please check |
What this PR does / why we need it:
This fixes a bug that broke all "shoot" and "cp" scope while refactoring the state management:
4b4c510#diff-abeb7e9d3448e309c3fc4a7cc959611852d5242b0fcf58540b063f494b131b04L41
The prop
shootItem
used to be passed toGTerminalShortcuts
component, this was removed but the component still relies on the object to be present for it to include the terminal shortcuts as seen here:dashboard/frontend/src/components/GTerminalShortcuts.vue
Lines 137 to 139 in 2b0f0ae
But
this.shootItem
was never set anymore in any parent component as prop:https://github.com/search?q=repo%3Agardener%2Fdashboard+%22%3Cg-terminal-shortcuts%22&type=code
this PR get the
shootItem
via theuseShootItem
composable the same way as in theGTerminalShortcut.vue
:4b4c510#diff-3925a06cd814d08ed6d969e9c3553128a5b66d2b5415c735374bd5b1fe2315efR132-R142
With this change all three scopes "garden", "cp" and "shoot" shortcuts are shown again correctly
Which issue(s) this PR fixes:
Special notes for your reviewer:
Release note: