Re: [pve-devel] [PATCH manager follow-up 2/2] ui: ceph status: add tooltip with details to warnings

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 3/2/23 14:15, Dominik Csapak wrote:
> one bug did occur when trying it out:
> 
> if i had my mouse over a warning, and then the warnings vanished all,
> the tooltip was still there and i could trigger it with hovering over
> the grid
> 
> so we want to clean the tooltips up on every load anyway,

Wouldn't it cause the tooltip to disappear, or at least flicker?

> or at least when the number of items is smaller than the position
> my mouse is over ;)
Yep

> 
> some comments inline
> 
> On 2/22/23 10:39, Aaron Lauterer wrote:
>> This is another step to make it easier for admins to discover more
>> information for a warning or problem that is shown in the Ceph health
>> panel.
>>
>> The length is limited to give a first glimpse. For the full details one
>> can click on the info/detail button.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>> Please use this follow up!
>>
>> I found a small bug just after sending it regarding the max length.
>> The length check and substring did have different values.
>>
>>   www/manager6/ceph/Status.js | 41 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>> index 45583f51..19a1243a 100644
>> --- a/www/manager6/ceph/Status.js
>> +++ b/www/manager6/ceph/Status.js
>> @@ -76,6 +76,47 @@ Ext.define('PVE.node.CephStatus', {
>>               trackRemoved: false,
>>               data: [],
>>               },
>> +
>> +            listeners: {
>> +            destroy: function() {
>> +                let view = this.getView();
>> +                if (view.tooltip) {
>> +                view.tooltip.destroy();
>> +                delete view.tooltip;
>> +                }
> 
> you could also do :
> 
> view.tooltip?.destroy();
> delete view.tooltip;
> 
> without the if
> 
> also the function is the same on itemmouseleave and destroy, so please
> refactor that into a function
> (since we also have to use it elsewhere too, see my comment at the top)

Okay, will do.

> 
>> +            },
>> +            itemmouseenter: function(record, item) {
>> +                let view = this.getView();
>> +                if (!view) {
>> +                return;
>> +                }
>> +                if (!item.data.detail) {
>> +                return;
>> +                }
>> +                let text = item.data.detail.trimStart().replaceAll('\n', 
>> '<br>');
> 
> any reason why we don't also use the <pre> tag here like in the popup?
> would at least be less computational intensive for long errors

I did try it with the pre tag, the problem then is, that the tooltip has a max 
length and the rest of the line is hidden. but that part contains important 
information as well. So I opted for a line break. Visibly not as pleasing, but 
it shows the full line.

> 
> also i'd check the length before replacing the newlines, otherwise
> it can happen that you cut the text of in the middle of a <br> tag ;)

Okay.

> 
> 
>> +                if (text.length > 500) {
>> +                    text = `${text.substring(0, 500)}…`;
> 
> nit: any special reason for the ellipsis glyph instead of 3 dots?
> not that it's a problem, just want to know

Because the ellipsis is the thing to use in such a situation? It is possible 
that my understanding is wrong. I have no problem with 3 dots as well.

> 
>> +                }
>> +                if (!view.tooltip) {
>> +                view.tooltip = Ext.create('Ext.tip.ToolTip', {
>> +                    target: view,
>> +                    trackMouse: true,
>> +                    dismissDelay: 0,
>> +                    tpl: '{text}',
>> +                    renderTo: Ext.getBody(),
>> +                });
>> +                }
>> +                view.tooltip.setData({ text });
>> +                view.tooltip.show();
>> +            },
>> +            itemmouseleave: function(record, item) {
>> +                let view = this.getView();
>> +                if (view.tooltip) {
>> +                view.tooltip.destroy();
>> +                delete view.tooltip;
>> +                }
>> +            },
>> +            },
>>               emptyText: gettext('No Warnings/Errors'),
>>               columns: [
>>               {
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel