[pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[Dominic Jäger]

Already talked about the first patch for #3058 with Thomas. Hope that this is
now acceptable.

The rest is rather to show what we could maybe do and how much code that would
require.

Dominic Jäger (4):
  ui: remote edit: Add tooltips
  ui: remote edit: Add asterisks
  ui: remote edit: Error as symbol to the right
  ui: remote edit: Change asterisk color

 www/Utils.js             | 15 +++++++++++++++
 www/window/RemoteEdit.js | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/4] ui: remote edit: Add tooltips

[Dominic Jäger]

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

diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
index 4a4d8114..74a9412b 100644
--- a/www/window/RemoteEdit.js
+++ b/www/window/RemoteEdit.js
@@ -39,6 +39,10 @@ Ext.define('PBS.window.RemoteEdit', {
 		cbind: {
 		    editable: '{isCreate}',
 		},
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip': gettext('A unique name to identify this remote.'),
+		},
 	    },
 	    {
 		xtype: 'proxmoxtextfield',
@@ -47,6 +51,10 @@ Ext.define('PBS.window.RemoteEdit', {
 		submitValue: false,
 		vtype: 'HostPort',
 		fieldLabel: gettext('Host'),
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip': gettext('The DNS or IP address of the remote, optionally with a port.'),
+		},
 		listeners: {
 		    change: function(field, newvalue) {
 			let host = newvalue;
@@ -93,6 +101,10 @@ Ext.define('PBS.window.RemoteEdit', {
 		allowBlank: false,
 		name: 'auth-id',
 		fieldLabel: gettext('Auth ID'),
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip': gettext('For example: admin@pbs.'),
+		},
 	    },
 	    {
 		xtype: 'textfield',
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] ui: remote edit: Add asterisks

[Dominic Jäger]

Mark fields that are required with an asterisk.

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

diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
index 74a9412b..391b10bc 100644
--- a/www/window/RemoteEdit.js
+++ b/www/window/RemoteEdit.js
@@ -35,6 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		fieldLabel: gettext('Remote'),
 		renderer: Ext.htmlEncode,
 		allowBlank: false,
+		afterLabelTextTpl: " *",
 		minLength: 4,
 		cbind: {
 		    editable: '{isCreate}',
@@ -47,6 +48,7 @@ Ext.define('PBS.window.RemoteEdit', {
 	    {
 		xtype: 'proxmoxtextfield',
 		allowBlank: false,
+		afterLabelTextTpl: " *",
 		name: 'hostport',
 		submitValue: false,
 		vtype: 'HostPort',
@@ -99,6 +101,7 @@ Ext.define('PBS.window.RemoteEdit', {
 	    {
 		xtype: 'proxmoxtextfield',
 		allowBlank: false,
+		afterLabelTextTpl: " *",
 		name: 'auth-id',
 		fieldLabel: gettext('Auth ID'),
 		autoEl: {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/4] ui: remote edit: Error as symbol to the right

[Dominic Jäger]

On error, a symbol appears to the right of the textfield.
Then every error message appears there.

This way users can
1. Hover over the error symbol to read the error message
2. Hover over the label or the field to see the regular tooltip

So this avoids the problem, that the error message hides the regular tooltip
when users hover over the textfield.

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

diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
index 391b10bc..7fb3a952 100644
--- a/www/window/RemoteEdit.js
+++ b/www/window/RemoteEdit.js
@@ -40,6 +40,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		cbind: {
 		    editable: '{isCreate}',
 		},
+		msgTarget: 'side',
 		autoEl: {
 		    tag: 'div',
 		    'data-qtip': gettext('A unique name to identify this remote.'),
@@ -53,6 +54,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		submitValue: false,
 		vtype: 'HostPort',
 		fieldLabel: gettext('Host'),
+		msgTarget: 'side',
 		autoEl: {
 		    tag: 'div',
 		    'data-qtip': gettext('The DNS or IP address of the remote, optionally with a port.'),
@@ -104,6 +106,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		afterLabelTextTpl: " *",
 		name: 'auth-id',
 		fieldLabel: gettext('Auth ID'),
+		msgTarget: 'side',
 		autoEl: {
 		    tag: 'div',
 		    'data-qtip': gettext('For example: admin@pbs.'),
@@ -114,6 +117,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		inputType: 'password',
 		fieldLabel: gettext('Password'),
 		name: 'password',
+		msgTarget: 'side',
 		cbind: {
 		    emptyText: '{passwordEmptyText}',
 		    allowBlank: '{!isCreate}',
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/4] ui: remote edit: Change asterisk color

[Dominic Jäger]

The asterisk symbolises required input. Therefore,
 1. Color it red
 2. Display an error quicktip on it
if and only if the error is about missing input.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
If somebody has a shorter idea, I'm all ears.

 www/Utils.js             | 15 +++++++++++++++
 www/window/RemoteEdit.js | 22 +++++++++++++++++++---
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/www/Utils.js b/www/Utils.js
index 14b58a25..1af8cddc 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -11,6 +11,21 @@ Ext.define('PBS.Utils', {
 
     dataStorePrefix: 'DataStore-',
 
+    getAsteriskTemplate: function() {
+	return new Ext.XTemplate(`<span id={id}-asterisk> *</span>`);
+    },
+
+    changeAsteriskColor: function(asterisk, error) {
+	let color = error.match(/required/) ? "red" : undefined;
+	let qtip = error.match(/required/) ? error : undefined;
+	asterisk.set({
+	    'data-errorqtip': qtip,
+	    style: {
+		color: color,
+	    },
+	});
+    },
+
     cryptmap: [
 	'none',
 	'mixed',
diff --git a/www/window/RemoteEdit.js b/www/window/RemoteEdit.js
index 7fb3a952..ce9a49b4 100644
--- a/www/window/RemoteEdit.js
+++ b/www/window/RemoteEdit.js
@@ -35,7 +35,7 @@ Ext.define('PBS.window.RemoteEdit', {
 		fieldLabel: gettext('Remote'),
 		renderer: Ext.htmlEncode,
 		allowBlank: false,
-		afterLabelTextTpl: " *",
+		afterLabelTextTpl: PBS.Utils.getAsteriskTemplate(),
 		minLength: 4,
 		cbind: {
 		    editable: '{isCreate}',
@@ -45,11 +45,17 @@ Ext.define('PBS.window.RemoteEdit', {
 		    tag: 'div',
 		    'data-qtip': gettext('A unique name to identify this remote.'),
 		},
+		listeners: {
+		    errorchange: function(labelable, error) {
+			let asterisk = Ext.get(`${labelable.ownerCt.id}-asterisk`);
+			PBS.Utils.changeAsteriskColor(asterisk, error);
+		    },
+		},
 	    },
 	    {
 		xtype: 'proxmoxtextfield',
 		allowBlank: false,
-		afterLabelTextTpl: " *",
+		afterLabelTextTpl: PBS.Utils.getAsteriskTemplate(),
 		name: 'hostport',
 		submitValue: false,
 		vtype: 'HostPort',
@@ -82,6 +88,10 @@ Ext.define('PBS.window.RemoteEdit', {
 			field.up('inputpanel').down('field[name=host]').setValue(host);
 			field.up('inputpanel').down('field[name=port]').setValue(port);
 		    },
+		    errorchange: function(labelable, error) {
+			let asterisk = Ext.get(`${labelable.id}-asterisk`);
+			PBS.Utils.changeAsteriskColor(asterisk, error);
+		    },
 		},
 	    },
 	    {
@@ -103,7 +113,7 @@ Ext.define('PBS.window.RemoteEdit', {
 	    {
 		xtype: 'proxmoxtextfield',
 		allowBlank: false,
-		afterLabelTextTpl: " *",
+		afterLabelTextTpl: PBS.Utils.getAsteriskTemplate(),
 		name: 'auth-id',
 		fieldLabel: gettext('Auth ID'),
 		msgTarget: 'side',
@@ -111,6 +121,12 @@ Ext.define('PBS.window.RemoteEdit', {
 		    tag: 'div',
 		    'data-qtip': gettext('For example: admin@pbs.'),
 		},
+		listeners: {
+		    errorchange: function(labelable, error) {
+			let asterisk = Ext.get(`${labelable.id}-asterisk`);
+			PBS.Utils.changeAsteriskColor(asterisk, error);
+		    },
+		},
 	    },
 	    {
 		xtype: 'textfield',
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[Dominik Csapak]

ok after taking a look at it, there are some problems

while i'd find the asterisk ok, the combination of patches
here do not really work together well.

when we have the error icon, we'd not need the asterisk,
since they both show the error or were they not intended
to be applied together?

also, the generic tooltip does always show on the whole
element for me, so with either the asterisk or the
icon, i still get multiple tooltips over one another

another problem is with the detection of the 'required'
error. while this may work on english, extjs
errors are also translated, though i noticed we do
not do this yet for pbs (for pve we do)
so filtering by 'required' cannot work for other languages

and last but not least:
would it not be possible to show the tooltip just on the
label?
e.g.:

---
fieldLabel: `<div qtip="sometext">${gettext('labeltext')}</div>'
---

a little bit less intuitive, but should not create overlapping
tooltips

another alternative would be to add a displayfield below the
field with an explanation?

or simply add a screenshot with a list of fields to the documentation?

On 1/19/21 12:09 PM, Dominic Jäger wrote:
> Already talked about the first patch for #3058 with Thomas. Hope that this is
> now acceptable.
> 
> The rest is rather to show what we could maybe do and how much code that would
> require.
> 
> Dominic Jäger (4):
>    ui: remote edit: Add tooltips
>    ui: remote edit: Add asterisks
>    ui: remote edit: Error as symbol to the right
>    ui: remote edit: Change asterisk color
> 
>   www/Utils.js             | 15 +++++++++++++++
>   www/window/RemoteEdit.js | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+)
> 

Re: [pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[Dominic Jäger]

Thanks for looking at it!

On Tue, Jan 26, 2021 at 11:34:54AM +0100, Dominik Csapak wrote:
> when we have the error icon, we'd not need the asterisk,
> since they both show the error
We could theoretically show the asterisk without any color, just to symbolise
"Required". But the error message shows this problem, too. So yes, it is
redundant in some way.

> or were they not intended to be applied together?
It was intended to be applicable together, but only to easily show the
possibilities together. Not because I absolutely want all of the patches to be
applied.

> also, the generic tooltip does always show on the whole
> element for me, so with either the asterisk or the
> icon, i still get multiple tooltips over one another
Same for me. But the error message should appear only if you hover over the
asterisk or the symbol. This means
1. if you hover over the error symbol, the error message is above the generic
   tooltip. Then you see what is wrong.
2. if you hover over the field, the error message vanishes and you see what you
   are supposed to enter into the field.

Without msgTarget: side the error tooltip should also appear if you hover over
the input field. Then you only see the generic tooltip when you hover over the
label, which is a little less intuitive than 2., I think.
> 
> another problem is with the detection of the 'required'
> error. while this may work on english, extjs
> errors are also translated, though i noticed we do
> not do this yet for pbs (for pve we do)
> so filtering by 'required' cannot work for other languages
Makes sense, I missed that.
> 
> and last but not least:
> would it not be possible to show the tooltip just on the
> label?
> e.g.:
> 
> ---
> fieldLabel: `<div qtip="sometext">${gettext('labeltext')}</div>'
> ---
I could not get this to work yet and I am not sure if it is possible?
The "Remote" field has xtype pmxDisplayEditField

> Ext.define('Proxmox.form.field.DisplayEdit', {
>     extend: 'Ext.form.FieldContainer',
>     alias: 'widget.pmxDisplayEditField',

and then fieldLabel is just a String, no HTML?
https://docs.sencha.com/extjs/6.0.1/classic/Ext.form.FieldContainer.html#cfg-fieldLabel

> 
> a little bit less intuitive, but should not create overlapping
> tooltips
This means we would have the same messages visible as currently?
1. Generic tool while hovering over label
2. Error message else (= hovering over field)

> 
> another alternative would be to add a displayfield below the
> field with an explanation?
Sounds good to me. Downside in comparison to tooltips would be limited amount
of text for the hints?

> 
> or simply add a screenshot with a list of fields to the documentation?
List of fields + what belongs into them? Isn't that quite much work to look up
"just" to fill out a field?

Re: [pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[Dominik Csapak]

On 1/27/21 11:55 AM, Dominic Jäger wrote:
> Thanks for looking at it!
> 
> On Tue, Jan 26, 2021 at 11:34:54AM +0100, Dominik Csapak wrote:
>> when we have the error icon, we'd not need the asterisk,
>> since they both show the error
> We could theoretically show the asterisk without any color, just to symbolise
> "Required". But the error message shows this problem, too. So yes, it is
> redundant in some way.

ok

> 
>> or were they not intended to be applied together?
> It was intended to be applicable together, but only to easily show the
> possibilities together. Not because I absolutely want all of the patches to be
> applied.

make sense

> 
>> also, the generic tooltip does always show on the whole
>> element for me, so with either the asterisk or the
>> icon, i still get multiple tooltips over one another
> Same for me. But the error message should appear only if you hover over the
> asterisk or the symbol. This means
> 1. if you hover over the error symbol, the error message is above the generic
>     tooltip. Then you see what is wrong.
> 2. if you hover over the field, the error message vanishes and you see what you
>     are supposed to enter into the field.
> 
> Without msgTarget: side the error tooltip should also appear if you hover over
> the input field. Then you only see the generic tooltip when you hover over the
> label, which is a little less intuitive than 2., I think.

the problem is rather that multiple tooltips above each other is ugly
and looks like something is wrong with the ui, so i'd like
to avoid that if possible

>>
>> another problem is with the detection of the 'required'
>> error. while this may work on english, extjs
>> errors are also translated, though i noticed we do
>> not do this yet for pbs (for pve we do)
>> so filtering by 'required' cannot work for other languages
> Makes sense, I missed that.

as we discussed off-list, we could simply check
if the field is empty instead

>>
>> and last but not least:
>> would it not be possible to show the tooltip just on the
>> label?
>> e.g.:
>>
>> ---
>> fieldLabel: `<div qtip="sometext">${gettext('labeltext')}</div>'
>> ---
> I could not get this to work yet and I am not sure if it is possible?
> The "Remote" field has xtype pmxDisplayEditField

works here (with the correct attribute ;) ):
---
fieldLabel: `<span data-qtip="some tooltip text">${gettext("Label")}</span>`
---

but i noticed it's even easier to do:

---
labelAttrTpl: 'data-qtip="Some Tooltip Text"',
---

puts that into the attributes of the whole label

> 
>> Ext.define('Proxmox.form.field.DisplayEdit', {
>>      extend: 'Ext.form.FieldContainer',
>>      alias: 'widget.pmxDisplayEditField',
> 
> and then fieldLabel is just a String, no HTML?
> https://docs.sencha.com/extjs/6.0.1/classic/Ext.form.FieldContainer.html#cfg-fieldLabel
> 
>>
>> a little bit less intuitive, but should not create overlapping
>> tooltips
> This means we would have the same messages visible as currently?
> 1. Generic tool while hovering over label
> 2. Error message else (= hovering over field)

yes, but no overlapping tooltips

> 
>>
>> another alternative would be to add a displayfield below the
>> field with an explanation?
> Sounds good to me. Downside in comparison to tooltips would be limited amount
> of text for the hints?
> 

yeah pretty much

>>
>> or simply add a screenshot with a list of fields to the documentation?
> List of fields + what belongs into them? Isn't that quite much work to look up
> "just" to fill out a field?
> 

but the admin usually needs that info only the first (few) times.
after that it just takes up space

Re: [pbs-devel] [PATCH proxmox-backup 0/4] remote edit: error message ideas

[Thomas Lamprecht]

On 27.01.21 14:57, Dominik Csapak wrote:
> On 1/27/21 11:55 AM, Dominic Jäger wrote:
>> Thanks for looking at it!
>>
>> On Tue, Jan 26, 2021 at 11:34:54AM +0100, Dominik Csapak wrote:
>>> when we have the error icon, we'd not need the asterisk,
>>> since they both show the error
>> We could theoretically show the asterisk without any color, just to symbolise
>> "Required". But the error message shows this problem, too. So yes, it is
>> redundant in some way.
> 
> ok

FWIW: one of my initial ideas which resulted Dominic to look into this
was separation of the field required and the other field invalid states
(out of range, not matching a regex, to long/short, ...).

The rational consisted at least of (may have forgotten something, it was
quite a bit ago):
* By default conflicts with other tooltips. Yes, other invalid tooltips
  do so too, but they require input which normally happens after initial
  evaluation of the fields and their meaning by the user
* less red and invalidity when opening a fresh inputpanel/editwindow
  while still keeping a visual hint about required fields
* it's also a well established pattern and very common in user-interface,
  thus not new for users.

(just FYI)

>>>
>>> ---
>>> fieldLabel: `<div qtip="sometext">${gettext('labeltext')}</div>'
>>> ---
>> I could not get this to work yet and I am not sure if it is possible?
>> The "Remote" field has xtype pmxDisplayEditField
> 
> works here (with the correct attribute ;) ):
> ---
> fieldLabel: `<span data-qtip="some tooltip text">${gettext("Label")}</span>`
> ---
> 
> but i noticed it's even easier to do:
> 
> ---
> labelAttrTpl: 'data-qtip="Some Tooltip Text"',
> ---
> 
> puts that into the attributes of the whole label

Doesn't this rather replaces the whole attributes, if any, and may thus have
some side effects (e.g., ARIA ones) depending on what the component had defined
in this template?

If that is guaranteed to be a non-issue this would be surely nicer.