Feat: Complete Kubectl Dialog Across Project,Workspace,Delete #15
Conversation
…nd-helper-dialogs
…nd-helper-dialogs
There was a problem hiding this comment.
Hey Fabian, awesome work! I’ve done a first review and it looks really cool, I really like this feature!
For the user-facing texts, let’s move them to en.json, since we’re using i18next. Just add your texts to the JSON file somewhere and get them with useTranslation. I’ve not marked all occasions, just a few, to give you a start.
src/components/Dialogs/KubectlCommandInfo/KubectlInfoButton.tsx
Outdated
Show resolved
Hide resolved
src/components/Dialogs/KubectlCommandInfo/KubectlInfoButton.tsx
Outdated
Show resolved
Hide resolved
src/components/Dialogs/KubectlCommandInfo/KubectlWorkspaceDialog.tsx
Outdated
Show resolved
Hide resolved
src/components/Dialogs/KubectlCommandInfo/KubectlProjectDialog.tsx
Outdated
Show resolved
Hide resolved
src/components/Dialogs/KubectlCommandInfo/KubectlBaseDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks much better, still a few things to do, especially with regard to naming. But overall good work!
Please delete the yarn.lock file and switch to npm to manage dependencies, e.g. npm i to install them etc.
As we discussed, let’s handle the refactoring in a follow-up task.
src/components/Dialogs/KubectlCommandInfo/KubectlDeleteMcpDialog.tsx
Outdated
Show resolved
Hide resolved
| import { useDialog } from '../../UseDialog'; | ||
|
|
||
| interface KubectlDeleteMcpProps { | ||
| projectName?: string; |
There was a problem hiding this comment.
Can this ever be undefined? Can there be MCPs without any project? If no, there’s no need to care about this use case
| projectName?: string; | |
| projectName: string; |
(also adjust ? further down the line)
yarn.lock
Outdated
| @@ -0,0 +1,4751 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
Please delete the yarn.lock file completely as we’ve moved to npm as a package manager.
| interface KubectlDialogProps extends ButtonPropTypes { | ||
| style?: CSSProperties; | ||
| } |
There was a problem hiding this comment.
The type for props should have the same name as the component.
It’s great that you extended the ButtonPropTypes but we should be more restrictive, as we don’t want to allow the consumer to pass any children (it’s we who set them). Also, there’s no need to explicitly mention style since it should already be optional.
Do you think it’s necessary to set style explicitly? Anything gained from specifying it as inline-block?
Anyway, here’s a suggestion on how we might write it
| interface KubectlDialogProps extends ButtonPropTypes { | |
| style?: CSSProperties; | |
| } | |
| interface KubectlInfoButtonProps extends Omit<ButtonPropTypes, 'children'> {} |
| isOpen: boolean; | ||
| } | ||
|
|
||
| export const KubectlProjectDialog = ({ |
There was a problem hiding this comment.
| export const KubectlProjectDialog = ({ | |
| export const KubectlCreateProjectDialog = ({ |
(also the props)
| project?: string; | ||
| } | ||
|
|
||
| export const KubectlWorkspaceDialog = ({ |
There was a problem hiding this comment.
| export const KubectlWorkspaceDialog = ({ | |
| export const KubectlCreateWorkspaceDialog = ({ |
(also the props)
| interface KubectlWorkspaceDialogProps { | ||
| onClose: () => void; | ||
| isOpen: boolean; | ||
| project?: string; |
There was a problem hiding this comment.
Can’t we assume that project is always defined? 🤔
4 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: