Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

["Shannon Sterz" <s.sterz@proxmox.com>]

On Tue Oct 29, 2024 at 1:22 PM CET, Christoph Heiss wrote:
> The patch title implies that it only adds a utility function, but the
> patch itself also includes usage for it. Something like
>
>   "fix #5787: ui: include guest name in confirmation dialog"
>
> or similar would probably be a better fit.
>
> On Tue, Oct 29, 2024 at 11:39:56AM GMT, Timothy Nicholson wrote:
> > Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> > ---
> > changes since v1:
> > - modified function signature
> > - removed unnecessary questionmark
>
> Link to the previous version on lore.proxmox.com is always nice to have
> in the notes, FWIW. But no hard feelings, just pure convenience if one
> wants to look at previous versions :^)
>
> >
> >  www/manager6/Utils.js            |  4 ++++
> >  www/manager6/lxc/CmdMenu.js      |  4 ++--
> >  www/manager6/lxc/Config.js       |  8 +++++---
> >  www/manager6/qemu/CmdMenu.js     |  4 ++--
> >  www/manager6/qemu/Config.js      | 14 ++++++++------
> >  www/manager6/window/GuestStop.js |  2 +-
> >  6 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> > index db86fa9a..493a635f 100644
> > --- a/www/manager6/Utils.js
> > +++ b/www/manager6/Utils.js
> > @@ -1960,6 +1960,10 @@ Ext.define('PVE.Utils', {
> >  	}
> >  	return languageCookie || Proxmox.defaultLang || 'en';
> >      },
> > +
> > +    format_guest_confirmation: function(taskType, vmid, guestName) {
>
> Maybe name it `format_guest_task_confirmation`, to really tie it to
> the task functionality?
>

just chiming in here as a reminder that our js guidelines want you to
use camelCase for new variables/functions [1].

[1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

> > +	return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
> > +    },
> >  },
> >
> >      singleton: true,
> > [..]
> > diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
> > index d0e40fc4..a15ff815 100644
> > --- a/www/manager6/lxc/Config.js
> > +++ b/www/manager6/lxc/Config.js
> > @@ -20,6 +20,8 @@ Ext.define('PVE.lxc.Config', {
> >  	    throw "no VM ID specified";
> >  	}
> >
> > +	var vmname = vm.name;
> > +
>
> Any particular reason for this variable binding? Using `vm.name` below
> directly would work too, if I'm not overlooking something. It's not
> really shorter anyway.
>
> In any case, please use `const` (or `let` if needed) for new variable
> declaration instead of `var`.
>
> >  	var template = !!vm.template;
> >
> >  	var running = !!vm.uptime;
> > [..]
> > diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> > index f28ee67b..1a9d1ba9 100644
> > --- a/www/manager6/qemu/Config.js
> > +++ b/www/manager6/qemu/Config.js
> > @@ -19,6 +19,8 @@ Ext.define('PVE.qemu.Config', {
> >  	    throw "no VM ID specified";
> >  	}
> >
> > +	var vmname = vm.name;
> > +
>
> Same as above.
>
> >  	var template = !!vm.template;
> >
> >  	var running = !!vm.uptime;
> > [..]
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel