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

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

[Maximiliano Sandoval]

Flat buttons are not obviously clickable without an icon.

The choice of icons was taken from pve-manager/www/manager6/Utils.js.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
Differences from v1:
  - Use the `fa-database` icon for local backups as suggested by Dominik
  
 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',
 	},
    ],
 });
-- 
2.39.2

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

[Thomas Lamprecht]

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',
>  	},
>     ],
>  });

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

[Dominik Csapak]

On 9/6/23 11:28, Thomas Lamprecht wrote:
>>
>> 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..

sorry that was probably my fault :S

Maximiliano asked me off list what i liked better, and i answered without too much thinking
of it (did not notice the v1 on the list at that point yet)

after thinking about it, i agree with thomas, that the 'folder' icon is even better,
is more consistent with pve (as that uses it for local storage too)

sorry again for the confusion


> 
>>    
>>   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',
>>   	},
>>      ],
>>   });
>