-
Notifications
You must be signed in to change notification settings - Fork 140
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
Implement Import/export button on frontend #81
Implement Import/export button on frontend #81
Conversation
Hi @196Ikuchil. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign |
d4e5e2e
to
81b5dd7
Compare
965f8ad
to
b26afec
Compare
Since #64 had merged, I have been rebased this branch. |
b26afec
to
d8b3358
Compare
I fixed the above errors. |
@@ -0,0 +1,42 @@ | |||
<template> |
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.
Let's rename this component because Export/Import features are not related to scheduler config.
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 didn't find a good name of it.
So, I renamed it to TopBar
.
I think this component will have more components in the future. e.g.) ResetButton
etc..
/ok-to-test |
@sanposhiho |
/unhold |
/retitle Implement Import/export button on frontend |
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.
Thanks. Left some comments.
|
||
export default defineComponent({ | ||
setup() { | ||
const data = reactive({ |
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.
Add watch
func and reset SelectedItem is data.dialog
is changed.
You can see how to use watch
func in ResourceBar component
https://github.com/kubernetes-sigs/kube-scheduler-simulator/blob/master/web/components/ResourceBar/ResourceBar.vue#L210
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 fixed that problem.
But this was caused by that file data being left in the input tag of the HTML.
So, I initialize and solve it by used to v-if
when the dialog is closed.
f80e7f2#diff-55b111ca13380e4133d565d2e89a7e59ba9dba0361de293248d0d05192a8236fR16
@sanposhiho Btw, the button will remain pressed after the dialog is closed. |
This is intended as it is how it works in MD2 spec which is the basis of vuetify 2.0 It has already been added as a prop called There is another method to resolve. But I think I'll keep it the current way if it is the basic. |
/retest |
@196Ikuchil Could you add |
@sanposhiho |
e2ce56b
to
10e6dd1
Compare
web/components/TopBar/TopBar.vue
Outdated
const buttonName = ["SchedulerConfiguration"]; | ||
const onClick = (bn: string) => { | ||
switch (bn) { | ||
case "SchedulerConfiguration": | ||
schedulerconfigurationstore.fetchSelected(); | ||
break; | ||
} | ||
}; |
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 this kinda abstraction is needed.
Let's make it simpler like before:
const onClick = () => {
schedulerconfigurationstore.fetchSelected();
};
Co-authored-by: Kensei Nakada <handbomusic@gmail.com>
Thanks, I forgot to fix it. (At the beginning of this work, I had an another design of that.) In addition, I refactored to split the button to another component. |
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.
Nice work!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 196Ikuchil, sanposhiho The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #28
Special notes for your reviewer:
/label tide/merge-method-squash
/hold