From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3E656910F for ; Wed, 8 Mar 2023 10:28:33 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E9FCE1E328 for ; Wed, 8 Mar 2023 10:28:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 8 Mar 2023 10:28:01 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B71B441C88 for ; Wed, 8 Mar 2023 10:28:00 +0100 (CET) Message-ID: <68c45f86-51f1-5656-b5d5-3f7b7d82b74c@proxmox.com> Date: Wed, 8 Mar 2023 10:27:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20230303155925.1142116-1-a.lauterer@proxmox.com> <20230303155925.1142116-3-a.lauterer@proxmox.com> From: Dominik Csapak In-Reply-To: <20230303155925.1142116-3-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.061 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [data.health] Subject: Re: [pve-devel] [PATCH v2 manager 2/2] ui: ceph status: add tooltip with details to warnings X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Mar 2023 09:28:33 -0000 nice, works really good now with the different interactions (button/container/changing store/etc) some comments inline (mostly minor stuff though) On 3/3/23 16:59, 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 > --- > changes since v1: > * only destroy it when leaving the whole grid, use show/hide for all > other situations > * factor out text handling of newlines and max lines > * factor out updating and destroying into their own functions > * add more listeners > * hide it when hovering the info button > * auto update it whenever the store is updated to either hide it > (nothing under the mouse) or set the content > > www/manager6/ceph/Status.js | 69 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js > index b223ab35..9de89df5 100644 > --- a/www/manager6/ceph/Status.js > +++ b/www/manager6/ceph/Status.js > @@ -76,6 +76,66 @@ Ext.define('PVE.node.CephStatus', { > trackRemoved: false, > data: [], > }, > + generateTooltipText: (text) => { > + text = text?.trimStart(); above you indicate that text might be undefined (with the 'text?.'), but below you don't to any checks again iff text can really be undefined, i'd do text = text?.trimStart() ?? ''; otherwise just drop the '?' > + if (text.length > 500) { > + text = `${text.substring(0, 500)}…`; > + } > + return text.replaceAll('\n', '
'); > + }, > + updateTooltip: (view, isLeave) => { > + if (!view.tooltip) { i was rather confused at first what 'view' actually is, but i found out it's the table view of the grid couldn't you use always the grid ('this') instead ? or is there some instance where 'this' isn't the grid? i think that would make the code a bit more readable, since you don't have to pass the view around > + return; > + } > + if (view.store.data.length - 1 < view.tooltip.gridIndex || isLeave) { > + view.tooltip.hide(); > + } else { > + let data = view.store.getData().items[view.tooltip.gridIndex].data; this probably only works by chance, since the view *also* has the store bound, normally we'd do 'me.getStore()' (which should work here with 'let me = this;' ) > + if (!data.detail) { > + return; > + } > + let text = view.up('#warnings').generateTooltipText(data.detail); also this could just be 'me.generateTooltipText(...)', no? > + view.tooltip.setData({ text }); > + } > + }, > + destroyTooltip(view) { > + view.tooltip?.destroy(); > + delete view.tooltip; > + }, > + > + listeners: { > + destroy: function() { > + let view = this.getView(); > + this.destroyTooltip(view); > + }, > + itemmouseenter: function(view, record, item, index) { > + if (!view) { > + return; > + } > + if (!record.data.detail) { > + return; > + } > + let text = this.generateTooltipText(record.data.detail); > + if (!view.tooltip) { > + view.tooltip = Ext.create('Ext.tip.ToolTip', { > + target: view, > + trackMouse: true, > + dismissDelay: 0, > + tpl: '{text}', > + renderTo: Ext.getBody(), > + }); > + } > + view.tooltip.gridIndex = index; > + view.tooltip.setData({ text }); > + view.tooltip.show(); > + }, > + itemmouseleave: function(view, record, item) { > + this.updateTooltip(view, true); > + }, > + containermouseout: function(view) { > + this.destroyTooltip(view); > + }, > + }, > emptyText: gettext('No Warnings/Errors'), > columns: [ > { > @@ -133,6 +193,14 @@ Ext.define('PVE.node.CephStatus', { > }], > }); > }, > + listeners: { > + mouseover: (view) => { > + view.up('#warnings').getView().tooltip.hide(); isn't here the 'view' actually the button? > + }, > + mouseout: (view) => { > + view.up('#warnings').getView().tooltip.show(); same here? > + }, > + }, > }, > }, > ], > @@ -286,6 +354,7 @@ Ext.define('PVE.node.CephStatus', { > me.down('#overallhealth').updateHealth(PVE.Utils.render_ceph_health(rec.data.health || {})); > // add errors to gridstore > me.down('#warnings').getStore().loadRawData(me.generateCheckData(rec.data.health || {}), false); > + me.down('#warnings').updateTooltip(me.down('#warnings').getView()); > > // update services > me.getComponent('services').updateAll(me.metadata || {}, rec.data);