-
Notifications
You must be signed in to change notification settings - Fork 74
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
adds VM (domain) title and description to UI #1001
Conversation
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.
src/libvirtApi/domain.js
Outdated
@@ -865,6 +865,36 @@ export function domainSetCpuMode({ | |||
], opts); | |||
} | |||
|
|||
export function domainSetTitle({ |
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.
domainSetTitle and domainSetDescription have essentially the same function body. Can you maybe parameterize it?
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 I will do it, no problem. Acually I thought the same, but tried to match the rest of the code and doing as little changes as possible. All other field seems to have their own functions and there are a lot cockpit.spawn methods which call virt-xml with basically the same arguments and I didn't want to refactor a whole file on my first contribution ;-).
src/components/vm/vmDetailsPage.jsx
Outdated
const vmTitleHeadline = ( | ||
<h2 className="vm-name"> | ||
{vm.title} | ||
<span style={{ fontSize: "var(--pf-global--FontSize--xl)", paddingLeft: "0.83em" }}> |
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 0.83 feels like a bit magic number.. Can you just maybe wrap this in a flex container with some spacing?
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
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.
The intention was having the same space ob both sides and the space in the right side is , according to the chrome debugger, determined by margin-block-end
of h2
wich is set to 0.83em. I could not find it as a css variable so I just hardcoded it. There are probably better ways but I'm not a React expert. The Flex spaceItems seems to put spaces on both sides.
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.
Additionally - I just realized that here we put the vm.name in parenthesis. Since most users don't use title, we defiintely want to have the name outside of parenthesis, when the user did not specify any title for the VM.
src/components/vm/vmDetailsPage.jsx
Outdated
@@ -87,15 +87,29 @@ export const VmDetailsPage = ({ | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
}, []); | |||
|
|||
const vmNameHeadline = ( |
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.
No need to keep this seperate. Just create a const like this:
const vmHeadline = (
<Flex spaceItems={{ default: 'spaceItemsSm' }} component='h2'>
{vm.title && <FlexItem>{vm.title}</FlexItem>}
<FlexItem style={{ fontSize: "var(--pf-global--FontSize--xl)">
{vm.name}
</span>
</Flex>
I did not test it, please adjust if needed.
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.
Great, this is how spaceItems is used. This code does not the same, but I now understand the idea and will change mine accordingly.
export const DescriptionModal = ({ vm }) => { | ||
const Dialogs = useDialogs(); | ||
const [error, setError] = useState({}); | ||
const [newDescription, setDescription] = useState(vm.description); |
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.
Just a nitpick - but we like to use the same name for the variable as the setter. Just use const [description, setDescription] = ...
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.
Makes sense. I will change it, but i will use [newDescription, setNewDescription]
to be consistent with the rename dialog code.
|
||
const defaultBody = ( | ||
<Form isHorizontal> | ||
<FormGroup id="title-model-select-group" label={_("New title")} |
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.
s/model/modal/g
const defaultBody = ( | ||
<Form isHorizontal> | ||
<FormGroup id="title-model-select-group" label={_("New title")} | ||
fieldId="rename-dialog-new-title" |
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.
It's not rename dialog;
s/rename/set-title/
Hey @Britz . Thanks a lot for this PR. Before we dive deeper into code review, do you mind explaining why did you decide to implement this specific feature? I'm asking, since I have not seen an issue or request for it before, so I don't really know what use case this solves, and what kind of additional information would a user input into the title/description which cannot be part of the VM's name. In cockpit-machines, we are quite careful about adding new features and expanding the UI, because we don't want the UI to become overcrowded with a lot of information, inputs, modals, etc.. So I'm just trying to find out what kind of use case this functionality solves, and how many users would use it..., so we can find the best place for this functionality in our UI :) Did you decide to implement it based on some issue or some request from somebody? Lastly, virt-manager also supports setting title/description. But a lot of virt-manager users use it for quasi-categories. Since virt-manager doesn't support VM categories, users input the category into the title/description, so they can categorize it that way. That's not ideal from virt-manager, and if this is the aim here too, I would like to avoid going exactly the same way. |
@KKoukiou: According to the test driven development, It would be great if you could this, I will try to help then. I've see that there are tests but no documentation on what and how to test and how to setup tests, so I just skipped it for now. I had not time to reverse engineer that by myself. |
@skobyda: Thanks for adding the screenshots. The rename dialog actually did not change, I will add additional screenshots to pinpoint the changes. Your question about why is quite simple. I simply implemented it, be cause we need that feature to better describe the purpose of the VM. Of cause you can code a lot of information into naming patterns, but i don't like extensively long domain names, especially when you need to type them regularly on shell, e.g., using To our setup/project: We are currently developing a small-scale on premises trusted research environment to do research on medical data in Germany (so strict laws like GDPR apply) and we use plain libvirt on our servers with an increasing amount of machines. We also tried oVirt, proxmox and even OpenStack and they do not fit our current requirements, so we need to use pure libvirt for now. Cockpit machine is a nice way to have a quick overview over the machines but it is far away from being a productive replacement of the underlaying console tools, thus we need both. If you are interested in our infrastructure, you can have a look at https://semla.dfki.de. |
I committed my changes and tried to fix all already mentioned issues, except the missing tests. Since I am a german, I only added the German translations of the additional text snippets. Should I add these the other languages without translation, such that other can easily spot and contribute to missing translations? How is the process there? |
@KKoukiou, according to the tests, I also need some help to understand the output. For me it is unclear how to work with the failed tests in the pull request. Looking at the logs, which are hard to read, it seems that the errors has nothing to do with my changes. |
@Britz I had a discussion yesterday with our designer (@garrett) and he believes adding these few extra options in the overview makes the UI rather cramped. He is working on a new design which will fit this in nicely, and then after I can pick up your commits to incorporate in the new design, add tests etc. Thanks for the contributions and sorry for the waiting time on this. I hope this is not super urgent. |
@KKoukiou That's great to here. It's in deed not the best design and really pragmatic. My first approach was to only put it in the header and add three field inside the rename dialogue, but that actually also fails, if the name of the vm does not change and thus it would always create errors if only the title or the description would be changed, which is not nice. Since it is easy to install from Git, we can also use the version in my branch until the feature is there. When you have final idea of how to incorporate this information, perhaps I can help to implement. I also has other required changes on my todo list, but I had not yet time to implement or to check if there are already issues. This includes, e.g., the name of the created vnet interface on the host in the network interface overview table, which is a very important information. |
I think that's a reasonable approach since it leads to fewer changes to the page. I think it's also discoverable: if people care about the names of their machines and want to improve them, they will see the additional options like title and description, and might change those instead of the id. I wouldn't use "Title" in the UI, but rather something like "Display name" or "Long name".
I am pretty sure we can make this work somehow. |
I have done my own version of this (mostly to get more familiar with the code in general): #1644. Let's continue there, ok? |
The libvirt domain XML has the title and description element which is not represented in the UI.
This commit adds, if available, the Title to the VM List in the form "<TITLE> ()" and adds Title and description to the VM detail page including the option to modify it.