[pve-devel] [PATCH v3 manager/widget-toolkit 0/2] ui: replace non-clickable checkboxes with icons

[pve-devel] [PATCH v3 manager/widget-toolkit 0/2] ui: replace non-clickable checkboxes with icons

[Lukas Wagner]

In our UI, we've been a bit inconsistent with the use of checkboxes/text
for `enabled` properties in table views. Looking through the UI, I've
found that the following UI elements use a checkbox UI control to
indicate wheter something is enabled or not:

 * backup job overview
 * APT repository overview
 * replication job overview

While looking sleek, the problem with this is that from a user's
perspective, a checkbox generally implies that it is operable by
clicking on it (which we allow in other places, to make the matter even
more confusing). Now, for the three UI elements mentioned above, I would
say it is a good thing that they are not manipulateable from the overview,
in order to avoid any accidental modifications.

In this patch series, the checkbox is replaced by a checkmark
(fa-check) icon if enabled, or by a minus/dash (fa-minus) icon if disabled.

Note: Firewall configuration also uses a checkbox, however there it is
possible to enable/disable elements by clicking on the checkbox.
As mentioned by Thomas in v1, this could be replaced by a more explicit
icon-based toggle button in a future series.

Changes v2 -> v3:
  * Fixed typo 'dataindex' -> 'dataIndex'
  * Use camel case for render function
  * Use arrow functions
  * Remove spurious newline

proxmox-widget-toolkit:
Lukas Wagner (1):
  repo view: replace non-clickable checkbox with icons

 src/Utils.js                | 2 ++
 src/node/APTRepositories.js | 6 ++----
 2 files changed, 4 insertions(+), 4 deletions(-)

pve-manager:
Lukas Wagner (1):
 ui: backup: replication: replace non-clickable checkbox with icons

 www/manager6/dc/Backup.js        | 7 +++----
 www/manager6/grid/Replication.js | 7 ++++---
 2 files changed, 7 insertions(+), 7 deletions(-)
-- 
2.30.2

[pve-devel] [PATCH v3 widget-toolkit 1/2] repo view: replace non-clickable checkbox with icons

[Lukas Wagner]

From a usability view, having a checkbox that is not clickable is pretty
misleading, especially if the visual style is exactly the same as in
other places in the UI where the checkbox is functional.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/Utils.js                | 2 ++
 src/node/APTRepositories.js | 6 ++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/Utils.js b/src/Utils.js
index ef0c2b8..a6dd314 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -100,6 +100,8 @@ utilities: {
 	return value;
     },
 
+    renderEnabledIcon: enabled => `<i class="fa fa-${enabled ? 'check' : 'minus'}"></i>`,
+
     language_array: function() {
 	let data = [['__default__', Proxmox.Utils.render_language('')]];
 	Ext.Object.each(Proxmox.Utils.language_map, function(key, value) {
diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index ce8f718..1fb627c 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -239,12 +239,10 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
 
     columns: [
 	{
-	    xtype: 'checkcolumn',
 	    header: gettext('Enabled'),
 	    dataIndex: 'Enabled',
-	    listeners: {
-		beforecheckchange: () => false, // veto, we don't want to allow inline change - to subtle
-	    },
+	    align: 'center',
+	    renderer: Proxmox.Utils.renderEnabledIcon,
 	    width: 90,
 	},
 	{
-- 
2.30.2

[pve-devel] [PATCH v3 manager 2/2] ui: backup: replication: replace non-clickable checkbox with icons

[Lukas Wagner]

From a usability view, having a checkbox that is not clickable is pretty
misleading, especially if the visual style is exactly the same as in
other places in the UI where the checkbox is functional.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/dc/Backup.js        | 6 ++----
 www/manager6/grid/Replication.js | 7 ++++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 9d305984..4d12e802 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -785,11 +785,9 @@ Ext.define('PVE.dc.BackupView', {
 		    header: gettext('Enabled'),
 		    width: 80,
 		    dataIndex: 'enabled',
-		    xtype: 'checkcolumn',
+		    align: 'center',
+		    renderer: Proxmox.Utils.renderEnabledIcon,
 		    sortable: true,
-		    disabled: true,
-		    disabledCls: 'x-item-enabled',
-		    stopSelection: false,
 		},
 		{
 		    header: gettext('ID'),
diff --git a/www/manager6/grid/Replication.js b/www/manager6/grid/Replication.js
index b17288b9..1e4e00fc 100644
--- a/www/manager6/grid/Replication.js
+++ b/www/manager6/grid/Replication.js
@@ -279,11 +279,12 @@ Ext.define('PVE.grid.ReplicaView', {
 
 	me.columns = [
 	    {
-		text: gettext('Enabled'),
+		header: gettext('Enabled'),
+		width: 80,
 		dataIndex: 'enabled',
-		xtype: 'checkcolumn',
+		align: 'center',
+		renderer: Proxmox.Utils.renderEnabledIcon,
 		sortable: true,
-		disabled: true,
 	    },
 	    {
 		text: 'ID',
-- 
2.30.2

[pve-devel] applied: [PATCH v3 manager 2/2] ui: backup: replication: replace non-clickable checkbox with icons

[Thomas Lamprecht]

On 26/01/2023 11:47, Lukas Wagner wrote:
> From a usability view, having a checkbox that is not clickable is pretty
> misleading, especially if the visual style is exactly the same as in
> other places in the UI where the checkbox is functional.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  www/manager6/dc/Backup.js        | 6 ++----
>  www/manager6/grid/Replication.js | 7 ++++---
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
>

applied, but expanded the calls to renderEnabledIcon for now to avoid doing
an immediate widget-toolkit bump + debian/control version dependency bump in
manager, thanks!

[pve-devel] applied: [PATCH v3 widget-toolkit 1/2] repo view: replace non-clickable checkbox with icons

[Thomas Lamprecht]

On 26/01/2023 11:47, Lukas Wagner wrote:
> From a usability view, having a checkbox that is not clickable is pretty
> misleading, especially if the visual style is exactly the same as in
> other places in the UI where the checkbox is functional.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/Utils.js                | 2 ++
>  src/node/APTRepositories.js | 6 ++----
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH v3 widget-toolkit 1/2] repo view: replace non-clickable checkbox with icons

[Lukas Wagner]

Thank you for your input!

On 1/27/23 12:09, Dominik Csapak wrote:
> i know i'm a bit late to the party, but couldn't we simply show nothing when a repo is not enabled?
> it's greyed-out anyway (though i'm not opposed to using the 'minus' icon here either)
> 

I have considered this option as well. IMO having the minus makes lists a bit clearer where all entities are disabled - for repos, this is probably never the case, but it could be for Backup/Replication jobs. It's just my preference though, I have no hard feelings about it.

-- 
- Lukas

Re: [pve-devel] [PATCH v3 widget-toolkit 1/2] repo view: replace non-clickable checkbox with icons

[Dominik Csapak]

On 1/26/23 11:47, Lukas Wagner wrote:
>  From a usability view, having a checkbox that is not clickable is pretty
> misleading, especially if the visual style is exactly the same as in
> other places in the UI where the checkbox is functional.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>   src/Utils.js                | 2 ++
>   src/node/APTRepositories.js | 6 ++----
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index ef0c2b8..a6dd314 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -100,6 +100,8 @@ utilities: {
>   	return value;
>       },
>   
> +    renderEnabledIcon: enabled => `<i class="fa fa-${enabled ? 'check' : 'minus'}"></i>`,
> +

i know i'm a bit late to the party, but couldn't we simply show nothing when a repo is not enabled?
it's greyed-out anyway (though i'm not opposed to using the 'minus' icon here either)