[pbs-devel] [PATCH proxmox-backup] Close #3058: ui: RemoteEdit: Add empty texts

[pbs-devel] [PATCH proxmox-backup] Close #3058: ui: RemoteEdit: Add empty texts

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 www/window/RemoteEdit.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
index 56a248e0..695ad422 100644
--- a/www/window/RemoteEdit.js
+++ b/www/window/RemoteEdit.js
@@ -24,7 +24,7 @@ Ext.define('PBS.window.RemoteEdit', {
 	me.method = name ? 'PUT' : 'POST';
 	me.autoLoad = !!name;
 	return {
-	    passwordEmptyText: me.isCreate ? '' : gettext('Unchanged'),
+	    passwordEmptyText: me.isCreate ? '•'.repeat(16) : gettext('Unchanged'),
 	};
     },
 
@@ -35,6 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		xtype: 'pmxDisplayEditField',
 		name: 'name',
 		fieldLabel: gettext('Remote'),
+		emptyText: 'new_remote',
 		renderer: Ext.htmlEncode,
 		allowBlank: false,
 		minLength: 4,
@@ -49,6 +50,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		submitValue: false,
 		vtype: 'HostPort',
 		fieldLabel: gettext('Host'),
+		emptyText: '192.168.10.10',
 		listeners: {
 		    change: function(field, newvalue) {
 			let host = newvalue;
@@ -95,6 +97,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		allowBlank: false,
 		name: 'userid',
 		fieldLabel: gettext('Userid'),
+		emptyText: 'root@pam',
 	    },
 	    {
 		xtype: 'textfield',
@@ -116,6 +119,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		    deleteEmpty: '{!isCreate}',
 		},
 		fieldLabel: gettext('Fingerprint'),
+		emptyText: '0f:04:9a:2d:48:23:52:5d:d5:f4:1d:9e:5d:37:8e:60:93:d8:b0:03:31:8c:5c:d5:49:ff:96:31:9e:c2:1d:74',
 	    },
 	    {
 		xtype: 'proxmoxtextfield',
@@ -124,6 +128,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		    deleteEmpty: '{!isCreate}',
 		},
 		fieldLabel: gettext('Comment'),
+		emptyText: gettext('A very long comment'),
 	    },
 	],
     },
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup] Close #3058: ui: RemoteEdit: Add empty texts

[Thomas Lamprecht]

Thanks for trying to improve user experience, I always appreciate that.
But, we normally use emptyText for defaults, or at least make it very
clear that they are not, if used as hint. You may want to switch those
over to tooltips with more complete sentences.

some more comments inline

On 06.10.20 12:10, Dominic Jäger wrote:
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>  www/window/RemoteEdit.js | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
> index 56a248e0..695ad422 100644
> --- a/www/window/RemoteEdit.js
> +++ b/www/window/RemoteEdit.js
> @@ -24,7 +24,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  	me.method = name ? 'PUT' : 'POST';
>  	me.autoLoad = !!name;
>  	return {
> -	    passwordEmptyText: me.isCreate ? '' : gettext('Unchanged'),
> +	    passwordEmptyText: me.isCreate ? '•'.repeat(16) : gettext('Unchanged'),
>  	};
>      },
>  
> @@ -35,6 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  		xtype: 'pmxDisplayEditField',
>  		name: 'name',
>  		fieldLabel: gettext('Remote'),
> +		emptyText: 'new_remote',

Suggests that this is the default, makes no sense IMO.

maybe add a "The unique ID for this remote" as tooltip and/or
change the fieldLabel to 'ID' instead.

>  		renderer: Ext.htmlEncode,
>  		allowBlank: false,
>  		minLength: 4,
> @@ -49,6 +50,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  		submitValue: false,
>  		vtype: 'HostPort',
>  		fieldLabel: gettext('Host'),
> +		emptyText: '192.168.10.10',

This suggests we use that IP as default, rather use a descriptive hint.
If it's long, then it could be also added as tooltip instead, which job
is to explain context better.


Could also change fieldLabel to 'Remote Address'

>  		listeners: {
>  		    change: function(field, newvalue) {
>  			let host = newvalue;
> @@ -95,6 +97,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  		allowBlank: false,
>  		name: 'userid',
>  		fieldLabel: gettext('Userid'),
> +		emptyText: 'root@pam',

Is this the default? As else it is wrong to show as such, if we do this
then we normally use something like "E.g., root@pam" or "For example, root@pam"
to indicate that this is not the default but a hint. A tooltip could work better
also here.

>  	    },
>  	    {
>  		xtype: 'textfield',
> @@ -116,6 +119,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  		    deleteEmpty: '{!isCreate}',
>  		},
>  		fieldLabel: gettext('Fingerprint'),
> +		emptyText: '0f:04:9a:2d:48:23:52:5d:d5:f4:1d:9e:5d:37:8e:60:93:d8:b0:03:31:8c:5c:d5:49:ff:96:31:9e:c2:1d:74',

no, why would yo u use a real FP here? Rather something which indicates what we expect here
but does not suggests a default..

"ab:cd:ef:..."

or like we do in the PVE "Add PBS" storage addition dialog.

>  	    },
>  	    {
>  		xtype: 'proxmoxtextfield',
> @@ -124,6 +128,7 @@ Ext.define('PBS.window.RemoteEdit', {
>  		    deleteEmpty: '{!isCreate}',
>  		},
>  		fieldLabel: gettext('Comment'),
> +		emptyText: gettext('A very long comment'),
>  	    },
>  	],
>      },
> 

Re: [pbs-devel] [PATCH proxmox-backup] Close #3058: ui: RemoteEdit: Add empty texts

[Dominic Jäger]

Thank you for looking at it so quickly!

On Tue, Oct 06, 2020 at 12:22:03PM +0200, Thomas Lamprecht wrote:
> Thanks for trying to improve user experience, I always appreciate that.
> But, we normally use emptyText for defaults, or at least make it very
> clear that they are not, if used as hint. You may want to switch those
> over to tooltips with more complete sentences.

Seems I checked too many online guides that suggest writing examples into
placeholders/emptyText and too little source code.

So far the only tooltips I have found on textfields are the "This field is
required" from allowBlank: false. The existing tooltips are all on some kind of
button and simply adding "tooltip: 'Test'," to a proxmoxtextfield doesn't do
anything?

=> I stuck to your other suggestion and made more descriptive labels with
either
 - "Example: <example>" or
 - a description


> On 06.10.20 12:10, Dominic Jäger wrote:
> > Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> > ---
> >  www/window/RemoteEdit.js | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
> > index 56a248e0..695ad422 100644
> > --- a/www/window/RemoteEdit.js
> > +++ b/www/window/RemoteEdit.js
> > @@ -24,7 +24,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  	me.method = name ? 'PUT' : 'POST';
> >  	me.autoLoad = !!name;
> >  	return {
> > -	    passwordEmptyText: me.isCreate ? '' : gettext('Unchanged'),
> > +	    passwordEmptyText: me.isCreate ? '•'.repeat(16) : gettext('Unchanged'),
> >  	};
> >      },
> >  
> > @@ -35,6 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  		xtype: 'pmxDisplayEditField',
> >  		name: 'name',
> >  		fieldLabel: gettext('Remote'),
> > +		emptyText: 'new_remote',
> 
> Suggests that this is the default, makes no sense IMO.
> 
> maybe add a "The unique ID for this remote" as tooltip and/or
> change the fieldLabel to 'ID' instead.
Changed the label and added "Example:". I think having some example here makes
sense to show that a single word/string is required and signs like underscores
are allowed.

> 
> >  		renderer: Ext.htmlEncode,
> >  		allowBlank: false,
> >  		minLength: 4,
> > @@ -49,6 +50,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  		submitValue: false,
> >  		vtype: 'HostPort',
> >  		fieldLabel: gettext('Host'),
> > +		emptyText: '192.168.10.10',
> 
> This suggests we use that IP as default, rather use a descriptive hint.
> If it's long, then it could be also added as tooltip instead, which job
> is to explain context better.
> 
> 
> Could also change fieldLabel to 'Remote Address'
Changed the label and added "Example:". Not sure about adding the port number as
it's optional.
> 
> >  		listeners: {
> >  		    change: function(field, newvalue) {
> >  			let host = newvalue;
> > @@ -95,6 +97,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  		allowBlank: false,
> >  		name: 'userid',
> >  		fieldLabel: gettext('Userid'),
> > +		emptyText: 'root@pam',
> 
> Is this the default? 
Explicitly specifying the user ID is required by GUI and CLI

 ~ proxmox-backup-manager remote create test --host 1.1.1.1 --password test
 Error: parameter verification errors

 parameter 'userid': parameter is missing and it is not optional.


> As else it is wrong to show as such, if we do this
> then we normally use something like "E.g., root@pam" or "For example, root@pam"
> to indicate that this is not the default but a hint. A tooltip could work better
> also here.
Chose "Example:" because
 - we can use that in every textfield of this window without exceeding its
   length
 - it is recommended in our technical writing guide
 - It is like that in the "Add PBS" panel

Additionally, changed the example to admin@pbs because that's what we
have in "Add PBS" storage addition dialog.

> 
> >  	    },
> >  	    {
> >  		xtype: 'textfield',
> > @@ -116,6 +119,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  		    deleteEmpty: '{!isCreate}',
> >  		},
> >  		fieldLabel: gettext('Fingerprint'),
> > +		emptyText: '0f:04:9a:2d:48:23:52:5d:d5:f4:1d:9e:5d:37:8e:60:93:d8:b0:03:31:8c:5c:d5:49:ff:96:31:9e:c2:1d:74',
> 
> no, why would yo u use a real FP here? Rather something which indicates what we expect here
> but does not suggests a default..
> 
> "ab:cd:ef:..."
> 
> or like we do in the PVE "Add PBS" storage addition dialog.
Changed to the exact same descriptions as in that dialog.
> 
> >  	    },
> >  	    {
> >  		xtype: 'proxmoxtextfield',
> > @@ -124,6 +128,7 @@ Ext.define('PBS.window.RemoteEdit', {
> >  		    deleteEmpty: '{!isCreate}',
> >  		},
> >  		fieldLabel: gettext('Comment'),
> > +		emptyText: gettext('A very long comment'),
> >  	    },
> >  	],
> >      },
> > 
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup] Close #3058: ui: RemoteEdit: Add empty texts

[Thomas Lamprecht]

On 07.10.20 11:53, Dominic Jäger wrote:
> Thank you for looking at it so quickly!
> 
> On Tue, Oct 06, 2020 at 12:22:03PM +0200, Thomas Lamprecht wrote:
>> Thanks for trying to improve user experience, I always appreciate that.
>> But, we normally use emptyText for defaults, or at least make it very
>> clear that they are not, if used as hint. You may want to switch those
>> over to tooltips with more complete sentences.
> 
> Seems I checked too many online guides that suggest writing examples into
> placeholders/emptyText and too little source code.

There was no need to look at the source, just use PVE's webinterface and you
see it everywhere..

Please, always try to check out the webinterface from a *user* POV and try to
use the rules there, before following some random UX guides online (there are
far to many, often conflicting).

I do not think throwing in just a plain "192.168.10.11" IP helps anybody more
than reading the label does, could rather make one think that this only accepts
IPs and not DNS host names.

> 
> So far the only tooltips I have found on textfields are the "This field is
> required" from allowBlank: false. The existing tooltips are all on some kind of
> button and simply adding "tooltip: 'Test'," to a proxmoxtextfield doesn't do
> anything?

There are the ExtJS generate tooltips, like if a fields format limit (regex,
vtype, maximum value, ...) does not applies, but there are also quite some
manually added ones.


Some ExtJS components (e.g., buttons) provide a config "tooltip" property, for
those which don't you can use something like:

autoEl: {
    tag: 'div',
    'data-qtip': gettext('Include volume in backup job'),
},


PS: we have over 30 defined tooltips in pve-manager alone, so they are far from
being a rare.

> 
> => I stuck to your other suggestion and made more descriptive labels with
> either
>  - "Example: <example>" or
>  - a description
> 
> 
>> On 06.10.20 12:10, Dominic Jäger wrote:
>>> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
>>> ---
>>>  www/window/RemoteEdit.js | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
>>> index 56a248e0..695ad422 100644
>>> --- a/www/window/RemoteEdit.js
>>> +++ b/www/window/RemoteEdit.js
>>> @@ -24,7 +24,7 @@ Ext.define('PBS.window.RemoteEdit', {
>>>  	me.method = name ? 'PUT' : 'POST';
>>>  	me.autoLoad = !!name;
>>>  	return {
>>> -	    passwordEmptyText: me.isCreate ? '' : gettext('Unchanged'),
>>> +	    passwordEmptyText: me.isCreate ? '•'.repeat(16) : gettext('Unchanged'),
>>>  	};
>>>      },
>>>  
>>> @@ -35,6 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
>>>  		xtype: 'pmxDisplayEditField',
>>>  		name: 'name',
>>>  		fieldLabel: gettext('Remote'),
>>> +		emptyText: 'new_remote',
>>
>> Suggests that this is the default, makes no sense IMO.
>>
>> maybe add a "The unique ID for this remote" as tooltip and/or
>> change the fieldLabel to 'ID' instead.
> Changed the label and added "Example:". I think having some example here makes
> sense to show that a single word/string is required and signs like underscores
> are allowed.

we never do that for user chosen IDs. Think of it that way:
If you'd have a form for registering a newborn baby, wouldn't it be weird
if there's a "Name: For example, Bobby" field?

> 
>>
>>>  		renderer: Ext.htmlEncode,
>>>  		allowBlank: false,
>>>  		minLength: 4,
>>> @@ -49,6 +50,7 @@ Ext.define('PBS.window.RemoteEdit', {
>>>  		submitValue: false,
>>>  		vtype: 'HostPort',
>>>  		fieldLabel: gettext('Host'),
>>> +		emptyText: '192.168.10.10',
>>
>> This suggests we use that IP as default, rather use a descriptive hint.
>> If it's long, then it could be also added as tooltip instead, which job
>> is to explain context better.
>>
>>
>> Could also change fieldLabel to 'Remote Address'
> Changed the label and added "Example:". Not sure about adding the port number as
> it's optional.

That's why a tooltip can be much more helpful, as there you can say that one
can add an optional port.

>>
>>>  		listeners: {
>>>  		    change: function(field, newvalue) {
>>>  			let host = newvalue;
>>> @@ -95,6 +97,7 @@ Ext.define('PBS.window.RemoteEdit', {
>>>  		allowBlank: false,
>>>  		name: 'userid',
>>>  		fieldLabel: gettext('Userid'),
>>> +		emptyText: 'root@pam',
>>
>> Is this the default? 
> Explicitly specifying the user ID is required by GUI and CLI
> 
>  ~ proxmox-backup-manager remote create test --host 1.1.1.1 --password test
>  Error: parameter verification errors
> 
>  parameter 'userid': parameter is missing and it is not optional.
> 
> 

FYI: That was a rhetoric question, as you using the emptyLabel like that
suggests it is.