On 3/23/20 10:50 AM, Stefan Reiter wrote:
> 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?
let for new, certain mixing is OK (we won't move forward else)
>
>>> 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.
>
thanks, FYI: Dominik is on vacation this week, so I'll try to give a v3
a faster look.
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
