From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Maximiliano Sandoval <m.sandoval@proxmox.com>,
pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH pmg-gui v2] js/BackupConfiguration: Add icons to tab buttons
Date: Wed, 6 Sep 2023 11:28:46 +0200 [thread overview]
Message-ID: <7070fc25-624b-4baa-a8e3-4542b0891d5f@proxmox.com> (raw)
In-Reply-To: <20230906065537.1475193-1-m.sandoval@proxmox.com>
You subject is breaking with our style guide for commit messages, namely:
> But, do not just paste the affected files, including (parts of the)
> path and maybe even the file ending, as tag! That has no use
> (already contained in diff stat) and just makes it harder to read.
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages
Am 06/09/2023 um 08:55 schrieb Maximiliano Sandoval:
> Flat buttons are not obviously clickable without an icon.
>
> The choice of icons was taken from pve-manager/www/manager6/Utils.js.
From where there? I'd guess the storageSchema, but with the v2 change
from "building" to "database" this seems now outdated?
Also, nitpicking now, using "from pve-manager's PVE.Utils module" could be
again nicer than some path.
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> Differences from v1:
> - Use the `fa-database` icon for local backups as suggested by Dominik
where has this been suggested? I see no review on the list..
Please keep reviews public and not in some silo, if you really want to talk
off list it can be fine, but the summary of that discussion needs to go
on the mailing lists, either by Dominik suggesting that or by you replying
to v1 that it's will be replaced by a v2 due to said suggestion from Dominik.
I'd find the folder icon a slightly better fit for something local, like we
use for local storage's in the storageSchema in Proxmox VE, but no hard
feelings from my side – just wondering why database over that..
>
> js/BackupConfiguration.js | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/js/BackupConfiguration.js b/js/BackupConfiguration.js
> index e21771f..7122822 100644
> --- a/js/BackupConfiguration.js
> +++ b/js/BackupConfiguration.js
> @@ -12,11 +12,13 @@ Ext.define('PMG.BackupConfiguration', {
> itemId: 'local',
> title: gettext('Local Backup/Restore'),
> xtype: 'pmgBackupRestore',
> + iconCls: 'fa fa-database',
> },
> {
> itemId: 'proxmoxbackupserver',
> title: 'Proxmox Backup Server',
> xtype: 'pmgPBSConfig',
> + iconCls: 'fa fa-floppy-o',
> },
> ],
> });
next prev parent reply other threads:[~2023-09-06 9:28 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-06 6:55 Maximiliano Sandoval
2023-09-06 9:28 ` Thomas Lamprecht [this message]
2023-09-06 9:33 ` Dominik Csapak
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=7070fc25-624b-4baa-a8e3-4542b0891d5f@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=m.sandoval@proxmox.com \
--cc=pmg-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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox