-
Notifications
You must be signed in to change notification settings - Fork 667
kubevirt: add Actions to VM detail #1906
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
kubevirt: add Actions to VM detail #1906
Conversation
f11ac82 to
e5bb40d
Compare
e5bb40d to
940c4e2
Compare
940c4e2 to
236f561
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.
Why aren't we using e.g. React.FC type here?
Also, using the props destructuring form is much cleaner:
const Foo: React.FC<FooProps> = ({ name, namespace }) => {};If you need direct access to props, simply use an alias when destructuring:
const Foo: React.FC<FooProps> = (props: { name, namespace }) => {};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.
Fixed, it was not updated.
But the props still have to be there because they are passed down to the DetailsPage
frontend/packages/kubevirt-plugin/src/components/vms/vm-details-page.tsx
Outdated
Show resolved
Hide resolved
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.
Please avoid too many acronyms in module names.
I'd suggest to name this directory selectors/vmi-migrations (either plural or singular) instead.
"VMI" is a quite common acronym, but "VMIM" isn't.
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.
you are right; changed to vmi-migration
236f561 to
87baf5c
Compare
|
/test e2e-aws-console-olm |
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.
Do we really need to export both menuActions and menuActionsCreator?
It seems that menuActionsCreator is the one who is supposed to interpret (execute) menuActions.
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.
yes we do. The use cases differ:
- vm list: the row already has status, etc computed so it just passes it to
menuActions - vm detail: Once we get detail page resources we compute the status, etc and pass it to all the actions (which is the only way how to pass logic to execute after loading resources before rendering actions)
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 think we should add type annotations, e.g. migrations is treated as an array but the signature doesn't reflect this fact.
I'm OK doing this in a follow-up. When adding new code, always try to make use of proper TypeScript.
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.
added the types. Although K8sResourceKind instead of a Migration because we don't have the type yet (waits for type generation PR)
87baf5c to
87190b8
Compare
|
/test frontend |
87190b8 to
c4f3153
Compare
|
/test e2e-aws |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: suomiy, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws |
depends on: