Re: [pmg-devel] [PATCH pmg-gui v2] js/BackupConfiguration: Add icons to tab buttons

[Thomas Lamprecht <t.lamprecht@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',
>  	},
>     ],
>  });