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

[pve-devel] [PATCH v2 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 the future series.

proxmox-widget-toolkit:

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

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

pve-manager:

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

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


-- 
2.30.2

[pve-devel] [PATCH v2 widget-toolkit 1/1] 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                | 6 ++++++
 src/node/APTRepositories.js | 8 ++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/Utils.js b/src/Utils.js
index ef0c2b8..5397fd9 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -100,6 +100,11 @@ utilities: {
 	return value;
     },
 
+    render_enabled_icon: function(value) {
+	let icon = value ? 'fa-check' : 'fa-minus';
+	return `<i class="fa ${icon}"></i>`;
+    },
+
     language_array: function() {
 	let data = [['__default__', Proxmox.Utils.render_language('')]];
 	Ext.Object.each(Proxmox.Utils.language_map, function(key, value) {
@@ -145,6 +150,7 @@ utilities: {
 	return value ? Proxmox.Utils.enabledText : Proxmox.Utils.disabledText;
     },
 
+
     format_expire: function(date) {
 	if (!date) {
 	    return Proxmox.Utils.neverText;
diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index ce8f718..c6b45a2 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -239,11 +239,11 @@ 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
+	    dataindex: 'Enabled',
+	    align: 'center',
+	    renderer: function(enabled, cell, record) {
+		return Proxmox.Utils.render_enabled_icon(record.data.Enabled);
 	    },
 	    width: 90,
 	},
-- 
2.30.2

[pve-devel] [PATCH v2 manager] 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        | 11 ++++++-----
 www/manager6/grid/Replication.js | 11 +++++++----
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 9d305984..8a306330 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -784,12 +784,13 @@ Ext.define('PVE.dc.BackupView', {
 		{
 		    header: gettext('Enabled'),
 		    width: 80,
-		    dataIndex: 'enabled',
-		    xtype: 'checkcolumn',
+		    dataindex: 'enabled',
+		    align: 'center',
+		    renderer: function(enabled, cell, record) {
+			return Proxmox.Utils.render_enabled_icon(record.data.enabled);
+		    },
 		    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..261c2755 100644
--- a/www/manager6/grid/Replication.js
+++ b/www/manager6/grid/Replication.js
@@ -279,11 +279,14 @@ Ext.define('PVE.grid.ReplicaView', {
 
 	me.columns = [
 	    {
-		text: gettext('Enabled'),
-		dataIndex: 'enabled',
-		xtype: 'checkcolumn',
+		header: gettext('Enabled'),
+		width: 80,
+		dataindex: 'enabled',
+		align: 'center',
+		renderer: function(enabled, cell, record) {
+		    return Proxmox.Utils.render_enabled_icon(record.data.enabled);
+		},
 		sortable: true,
-		disabled: true,
 	    },
 	    {
 		text: 'ID',
-- 
2.30.2

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

[Thomas Lamprecht]

Am 23/01/2023 um 14:17 schrieb 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                | 6 ++++++
>  src/node/APTRepositories.js | 8 ++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index ef0c2b8..5397fd9 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -100,6 +100,11 @@ utilities: {
>  	return value;
>      },
>  
> +    render_enabled_icon: function(value) {
> +	let icon = value ? 'fa-check' : 'fa-minus';
> +	return `<i class="fa ${icon}"></i>`;
> +    },

I know casing is all over the place, but ExtJS uses camelCase and we interact
with it a lot, so I'd favor adding new stuff as camelCase over, e.g., snake_case.

For this here an arrow function might be worth it, e.g.:

renderEnabledIcon: enabled => `<i class="fa fa-${enabled ? 'check' : 'minus'}"></i>`,

btw. maybe it's really time to start a "Format" or "Render" class and move all
those renderer out from Utils; but the burden of doing so really shouldn't be the
one from this series.

> +
>      language_array: function() {
>  	let data = [['__default__', Proxmox.Utils.render_language('')]];
>  	Ext.Object.each(Proxmox.Utils.language_map, function(key, value) {
> @@ -145,6 +150,7 @@ utilities: {
>  	return value ? Proxmox.Utils.enabledText : Proxmox.Utils.disabledText;
>      },
>  
> +

spurious new line added.

>      format_expire: function(date) {
>  	if (!date) {
>  	    return Proxmox.Utils.neverText;
> diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
> index ce8f718..c6b45a2 100644
> --- a/src/node/APTRepositories.js
> +++ b/src/node/APTRepositories.js
> @@ -239,11 +239,11 @@ 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
> +	    dataindex: 'Enabled',
> +	    align: 'center',
> +	    renderer: function(enabled, cell, record) {
> +		return Proxmox.Utils.render_enabled_icon(record.data.Enabled);
>  	    },

could be a one liner without losing clarity:

renderer: ({data}) => Proxmox.Utils.renderEnabledIcon(data.Enabled),

>  	    width: 90,
>  	},
 

Re: [pve-devel] [PATCH v2 manager] ui: backup: replication: replace non-clickable checkbox with icons

[Thomas Lamprecht]

Am 23/01/2023 um 14:17 schrieb 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        | 11 ++++++-----
>  www/manager6/grid/Replication.js | 11 +++++++----
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 9d305984..8a306330 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -784,12 +784,13 @@ Ext.define('PVE.dc.BackupView', {
>  		{
>  		    header: gettext('Enabled'),
>  		    width: 80,
> -		    dataIndex: 'enabled',
> -		    xtype: 'checkcolumn',
> +		    dataindex: 'enabled',

you break setting dataIndex by writing it all lowercase ?!

> +		    align: 'center',
> +		    renderer: function(enabled, cell, record) {
> +			return Proxmox.Utils.render_enabled_icon(record.data.enabled);
> +		    },


with data index fixed, this could be:


renderer: Proxmox.Utils.renderEnabledIcon,

Or if you really want to avoid passing the remaining params to the renderer:

renderer: enabled => Proxmox.Utils.renderEnabledIcon(enabled),

but normally that's really not a problem for renders.

>  		    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..261c2755 100644
> --- a/www/manager6/grid/Replication.js
> +++ b/www/manager6/grid/Replication.js
> @@ -279,11 +279,14 @@ Ext.define('PVE.grid.ReplicaView', {
>  
>  	me.columns = [
>  	    {
> -		text: gettext('Enabled'),
> -		dataIndex: 'enabled',
> -		xtype: 'checkcolumn',
> +		header: gettext('Enabled'),
> +		width: 80,
> +		dataindex: 'enabled',

same here w.r.t. breaking dataIndex

> +		align: 'center',
> +		renderer: function(enabled, cell, record) {
> +		    return Proxmox.Utils.render_enabled_icon(record.data.enabled);
> +		},

same here w.r.t. shorter direct renderer

>  		sortable: true,
> -		disabled: true,
>  	    },
>  	    {
>  		text: 'ID',

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

[Thomas Lamprecht]

Am 24/01/2023 um 16:24 schrieb Thomas Lamprecht:
>> -	    dataIndex: 'Enabled',
>> -	    listeners: {
>> -		beforecheckchange: () => false, // veto, we don't want to allow inline change - to subtle
>> +	    dataindex: 'Enabled',
>> +	    align: 'center',
>> +	    renderer: function(enabled, cell, record) {
>> +		return Proxmox.Utils.render_enabled_icon(record.data.Enabled);
>>  	    },
> could be a one liner without losing clarity:
> 
> renderer: ({data}) => Proxmox.Utils.renderEnabledIcon(data.Enabled),
> 

Actually, as in my reply to 2/2, you break setting dataIndex by writing it
all lowercase, as otherwise you should be able to configure the renderer
function directly, without intermediate call

Re: [pve-devel] [PATCH v2 manager] ui: backup: replication: replace non-clickable checkbox with icons

[Lukas Wagner]

On 1/24/23 16:28, Thomas Lamprecht wrote:
> 
> you break setting dataIndex by writing it all lowercase ?!
> 
>> +		    align: 'center',
>> +		    renderer: function(enabled, cell, record) {
>> +			return Proxmox.Utils.render_enabled_icon(record.data.enabled);
>> +		    },
> 
> 
> with data index fixed, this could be:
> 
> 
> renderer: Proxmox.Utils.renderEnabledIcon,
> 
> Or if you really want to avoid passing the remaining params to the renderer:
> 
> renderer: enabled => Proxmox.Utils.renderEnabledIcon(enabled),
> 
> but normally that's really not a problem for renders.
> 

Ooooh, and I kept wondering why `renderer` did not behave as in other places.
Missspelled `dataIndex` must have snuck in as a copy-paste error, thanks for pointing this out.


-- 
- Lukas