[pve-devel] [PATCH v2 manager 1/2] gui/cluster: add CorosyncLinkEdit component to support up to 8 links

[Stefan Reiter]

On 19/03/2020 15:06, Dominik Csapak wrote:
> overall looks good, and most works, comments inline
> 
> the only 'real' issue i found is with ipv6 networks
> (they do not get auto added to the links)
> 

Thanks for the review, I'll send a fixed v3 soon!

> On 3/17/20 12:11 PM, Stefan Reiter wrote
>> -        ring1Possible: false,
>>           ip: '',
>>           clusterName: ''
>>           };
>> -        var totem = {};
>>           if (!(joinInfo && joinInfo.totem)) {
>>           field.valid = false;
>> +        linkEditor.setLinks([]);
>> +        linkEditor.setInfoText();
>>           } else {
>> -        var ring0Needed = false;
>> -        if (joinInfo.ring_addr !== undefined) {
>> -            ring0Needed = joinInfo.ring_addr[0] !== joinInfo.ipAddress;
>> +        var links = [];
>> +        var interfaces = joinInfo.totem['interface'];
> 
> nit: i'd rather use 'let' whenever possible for new code
> 

I used it here to not mix-and-match var and let in existing functions. 
Is this the intent, or should I also change other 'var's over to let then?

>>       columnB: [
>>           {
>>           xtype: 'textfield',
>>           fieldLabel: gettext('Fingerprint'),
>> +        style: 'margin-top: -10px',
> 
> same as for 'padding-top'
> also, why '-10px' ?
> 
> also, the whole layout seems kinda wonky (at least with
> the language set to german), the password field
> is higher up that the address field...
> 

Hadn't tested in other languages... Probably should have ;)

The reason for the '-10px' was to avoid exactly that issue in the 
English layout, where without it some text fields would mysteriously be 
misaligned. I'll try and figure out how to fix this correctly and for 
all localizations.