-
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
Changes from 1 commit
dbe8909
ace05db
fce1c87
c928110
90864c9
f7ef41b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import React, { useState } from 'react'; | ||
import cockpit from 'cockpit'; | ||
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js"; | ||
import { Form, FormGroup } from "@patternfly/react-core/dist/esm/components/Form/index.js"; | ||
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js"; | ||
import { Modal } from "@patternfly/react-core/dist/esm/components/Modal/index.js"; | ||
import { useDialogs } from 'dialogs.jsx'; | ||
|
||
import { ModalError } from 'cockpit-components-inline-notification.jsx'; | ||
import { domainSetDescription } from '../../../libvirtApi/domain.js'; | ||
|
||
const _ = cockpit.gettext; | ||
|
||
export const DescriptionModal = ({ vm }) => { | ||
const Dialogs = useDialogs(); | ||
const [error, setError] = useState({}); | ||
const [newDescription, setDescription] = useState(vm.description); | ||
const [isLoading, setIsLoading] = useState(false); | ||
|
||
function save() { | ||
setIsLoading(true); | ||
domainSetDescription({ | ||
name: vm.name, | ||
id: vm.id, | ||
connectionName: vm.connectionName, | ||
description: newDescription | ||
}).then(Dialogs.close, exc => { | ||
setIsLoading(false); | ||
setError({ dialogError: _("Description could not be changed"), dialogErrorDetail: exc.message }); | ||
}); | ||
} | ||
|
||
const defaultBody = ( | ||
<Form isHorizontal> | ||
<FormGroup id="description-model-select-group" label={_("New description")} | ||
fieldId="rename-dialog-new-description" | ||
> | ||
<TextInput id='rename-dialog-new-description' | ||
value={newDescription} | ||
onChange={setDescription} /> | ||
</FormGroup> | ||
</Form> | ||
); | ||
|
||
return ( | ||
<Modal position="top" variant="small" isOpen onClose={Dialogs.close} | ||
title={cockpit.format(_("Set description of $0"), vm.name)} | ||
footer={ | ||
<> | ||
<Button variant='primary' isDisabled={isLoading} isLoading={isLoading} id="description-config-dialog-apply" onClick={save}> | ||
{_("Save")} | ||
</Button> | ||
<Button variant='link' onClick={Dialogs.close}> | ||
{_("Cancel")} | ||
</Button> | ||
</> | ||
}> | ||
<> | ||
{error && error.dialogError && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />} | ||
{defaultBody} | ||
</> | ||
</Modal> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import React, { useState } from 'react'; | ||
import cockpit from 'cockpit'; | ||
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js"; | ||
import { Form, FormGroup } from "@patternfly/react-core/dist/esm/components/Form/index.js"; | ||
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js"; | ||
import { Modal } from "@patternfly/react-core/dist/esm/components/Modal/index.js"; | ||
import { useDialogs } from 'dialogs.jsx'; | ||
|
||
import { ModalError } from 'cockpit-components-inline-notification.jsx'; | ||
import { domainSetTitle } from '../../../libvirtApi/domain.js'; | ||
|
||
const _ = cockpit.gettext; | ||
|
||
export const TitleModal = ({ vm }) => { | ||
const Dialogs = useDialogs(); | ||
const [error, setError] = useState({}); | ||
const [newTitle, setTitle] = useState(vm.title); | ||
const [isLoading, setIsLoading] = useState(false); | ||
|
||
function save() { | ||
setIsLoading(true); | ||
domainSetTitle({ | ||
name: vm.name, | ||
id: vm.id, | ||
connectionName: vm.connectionName, | ||
title: newTitle | ||
}).then(Dialogs.close, exc => { | ||
setIsLoading(false); | ||
setError({ dialogError: _("Title could not be changed"), dialogErrorDetail: exc.message }); | ||
}); | ||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. s/model/modal/g |
||
fieldId="rename-dialog-new-title" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not rename dialog; s/rename/set-title/ |
||
> | ||
<TextInput id='rename-dialog-new-title' | ||
value={newTitle} | ||
onChange={setTitle} /> | ||
</FormGroup> | ||
</Form> | ||
); | ||
|
||
return ( | ||
<Modal position="top" variant="small" isOpen onClose={Dialogs.close} | ||
title={cockpit.format(_("Set title of $0"), vm.name)} | ||
footer={ | ||
<> | ||
<Button variant='primary' isDisabled={isLoading} isLoading={isLoading} id="title-config-dialog-apply" onClick={save}> | ||
{_("Save")} | ||
</Button> | ||
<Button variant='link' onClick={Dialogs.close}> | ||
{_("Cancel")} | ||
</Button> | ||
</> | ||
}> | ||
<> | ||
{error && error.dialogError && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />} | ||
{defaultBody} | ||
</> | ||
</Modal> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. No need to keep this seperate. Just create a const like this:
I did not test it, please adjust if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
<h2 className="vm-name">{vm.name}</h2> | ||
); | ||
|
||
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 commentThe 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
({vm.name}) | ||
</span> | ||
</h2> | ||
); | ||
|
||
const vmActionsPageSection = ( | ||
<PageSection className="actions-pagesection" variant={PageSectionVariants.light}> | ||
<div className="vm-top-panel" data-vm-transient={!vm.persistent}> | ||
<h2 className="vm-name">{vm.name}</h2> | ||
{vm.title ? vmTitleHeadline : vmNameHeadline} | ||
<VmActions vm={vm} | ||
config={config} | ||
onAddErrorNotification={onAddErrorNotification} | ||
isDetailsPage /> | ||
</div> | ||
{ vm.description && <h4>{vm.description}</h4> } | ||
</PageSection> | ||
); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -865,6 +865,36 @@ export function domainSetCpuMode({ | |
], opts); | ||
} | ||
|
||
export function domainSetTitle({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 ;-). |
||
name, | ||
id: objPath, | ||
connectionName, | ||
title, | ||
}) { | ||
const opts = { err: "message", environ: ['LC_ALL=C'] }; | ||
if (connectionName === 'system') | ||
opts.superuser = 'try'; | ||
|
||
return cockpit.spawn([ | ||
'virt-xml', '-c', `qemu:///${connectionName}`, '--metadata', `title="${title}"`, name, '--edit' | ||
], opts); | ||
} | ||
|
||
export function domainSetDescription({ | ||
name, | ||
id: objPath, | ||
connectionName, | ||
description, | ||
}) { | ||
const opts = { err: "message", environ: ['LC_ALL=C'] }; | ||
if (connectionName === 'system') | ||
opts.superuser = 'try'; | ||
|
||
return cockpit.spawn([ | ||
'virt-xml', '-c', `qemu:///${connectionName}`, '--metadata', `description="${description}"`, name, '--edit' | ||
], opts); | ||
} | ||
|
||
export function domainSetMemoryBacking({ connectionName, vmName, type }) { | ||
const options = { err: "message" }; | ||
if (connectionName === "system") | ||
|
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.