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.nichol...@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
