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

[Aaron Lauterer]

On 3/8/23 10:27, Dominik Csapak wrote:
> 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:
[...]
>> 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 '?'


hmm yeah, AFAICT there are already checks in place before we even call 
generateTooltipText -> dropping the '?'

> 
>> +            if (text.length > 500) {
>> +                text = `${text.substring(0, 500)}…`;
>> +            }
>> +            return text.replaceAll('\n', '<br>');
>> +            },
>> +            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

as discussed off-list: arrow functions do not set 'this' in the same context -> 
switch to regular 'updateTooltip: function(..) {' instead of arrow functions and 
in other places too.
> 

[...]