From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Hannes Laimer <h.laimer@proxmox.com>,
pbs-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH proxmox-widget-toolkit 3/5] ui: added possibility to show a small note in SafeRemove dialog
Date: Tue, 18 Aug 2020 19:59:13 +0200 [thread overview]
Message-ID: <8706f161-b1b0-f96b-d9fb-d53da07639ff@proxmox.com> (raw)
In-Reply-To: <20200818084023.54780-4-h.laimer@proxmox.com>
general note, while I find tags in the comment subject nice, this doesn't really
adds value as everything in widget toolkit is "ui" :) We only use that in things
like pve-manager or proxmox-backup-server as there API, UI and possibly other stuff
are shared in the same repo. You could use:
"safe destroy: add possibility to show a small extra note"
On 18.08.20 10:40, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/window/SafeRemove.js | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/src/window/SafeRemove.js b/src/window/SafeRemove.js
> index c541272..c3fc7dd 100644
> --- a/src/window/SafeRemove.js
> +++ b/src/window/SafeRemove.js
...
> @@ -173,6 +199,8 @@ Ext.define('Proxmox.window.SafeRemove', {
> msg = Proxmox.Utils.format_task_description('cephdestroypool', item.id);
> } else if (item.type === 'Image') {
> msg = Proxmox.Utils.format_task_description('unknownimgdel', item.id);
> + } else if (item.type === 'Dir') {
> + msg = `${gettext('Directory')} ${item.id} - ${gettext('Remove')}`
few issues:
1. misses a semicolon - did you even built it once? I'd guess that eslint complains about
it then.
2. has nothing to do with this patch, should be separate
3. why not a format task description? In combination with my comments on patch 1/5 that
could be much nicer, avoiding this clunky if/else block completely.
> } else {
> throw "unknown item type specified";
> }
>
WARNING: multiple messages have this Message-ID
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Hannes Laimer <h.laimer@proxmox.com>,
pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 3/5] ui: added possibility to show a small note in SafeRemove dialog
Date: Tue, 18 Aug 2020 19:59:13 +0200 [thread overview]
Message-ID: <8706f161-b1b0-f96b-d9fb-d53da07639ff@proxmox.com> (raw)
In-Reply-To: <20200818084023.54780-4-h.laimer@proxmox.com>
general note, while I find tags in the comment subject nice, this doesn't really
adds value as everything in widget toolkit is "ui" :) We only use that in things
like pve-manager or proxmox-backup-server as there API, UI and possibly other stuff
are shared in the same repo. You could use:
"safe destroy: add possibility to show a small extra note"
On 18.08.20 10:40, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/window/SafeRemove.js | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/src/window/SafeRemove.js b/src/window/SafeRemove.js
> index c541272..c3fc7dd 100644
> --- a/src/window/SafeRemove.js
> +++ b/src/window/SafeRemove.js
...
> @@ -173,6 +199,8 @@ Ext.define('Proxmox.window.SafeRemove', {
> msg = Proxmox.Utils.format_task_description('cephdestroypool', item.id);
> } else if (item.type === 'Image') {
> msg = Proxmox.Utils.format_task_description('unknownimgdel', item.id);
> + } else if (item.type === 'Dir') {
> + msg = `${gettext('Directory')} ${item.id} - ${gettext('Remove')}`
few issues:
1. misses a semicolon - did you even built it once? I'd guess that eslint complains about
it then.
2. has nothing to do with this patch, should be separate
3. why not a format task description? In combination with my comments on patch 1/5 that
could be much nicer, avoiding this clunky if/else block completely.
> } else {
> throw "unknown item type specified";
> }
>
next prev parent reply other threads:[~2020-08-18 17:59 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-18 8:40 [pve-devel] [PATCH series 0/5] removal of directories in PBS WebUI Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 8:40 ` [pve-devel] [PATCH proxmox-widget-toolkit 1/5] ui refactoring: refactored SafeDestroy from pve-manager into proxmox-widget-toolkit Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 17:40 ` [pve-devel] " Thomas Lamprecht
2020-08-18 17:40 ` [pbs-devel] " Thomas Lamprecht
2020-08-18 8:40 ` [pve-devel] [PATCH pve-manager 2/5] ui refactoring: SafeDestroy moved into widgettoolkit + adjusted usages Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 8:40 ` [pve-devel] [PATCH proxmox-widget-toolkit 3/5] ui: added possibility to show a small note in SafeRemove dialog Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 17:59 ` Thomas Lamprecht [this message]
2020-08-18 17:59 ` [pbs-devel] [pve-devel] " Thomas Lamprecht
2020-08-18 8:40 ` [pve-devel] [PATCH proxmox-backup 4/5] api2: DatastoreMountInfo now also contains the name of the mount Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 8:40 ` [pve-devel] [PATCH proxmox-backup 5/5] ui: added possiblity to remove directories/mount-units in the WebUI Hannes Laimer
2020-08-18 8:40 ` [pbs-devel] " Hannes Laimer
2020-08-18 17:50 ` [pve-devel] " Thomas Lamprecht
2020-08-18 17:50 ` [pbs-devel] " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8706f161-b1b0-f96b-d9fb-d53da07639ff@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=h.laimer@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal