On Fri Mar 28, 2025 at 6:13 PM CET, Gabriel Goller wrote: [..] > diff --git a/www/manager6/sdn/fabrics/openfabric/NodeEdit.js > b/www/manager6/sdn/fabrics/openfabric/NodeEdit.js > new file mode 100644 > index 000000000000..f2d204c22542 > --- /dev/null > +++ b/www/manager6/sdn/fabrics/openfabric/NodeEdit.js > @@ -0,0 +1,205 @@ > +Ext.define('PVE.sdn.Fabric.OpenFabric.Node.InputPanel', { > + extend: 'Proxmox.panel.InputPanel', > + > + viewModel: {}, > + > + isCreate: undefined, > + loadClusterInterfaces: undefined, > + > + interface_selector: undefined, > + node_not_accessible_warning: undefined,
All new variables should be camelCase [0] here too, as noted in a later patch. [0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing > + [..] > + initComponent: function() { > + let me = this; > + me.interface_selector = > Ext.create('PVE.sdn.Fabric.OpenFabric.InterfacePanel', { > + name: 'interfaces', > + parentClass: me.isCreate ? me : undefined, > + }); > + me.items = [ > + { > + xtype: 'pveNodeSelector', > + reference: 'nodeselector', > + fieldLabel: gettext('Node'), > + labelWidth: 120, > + name: 'node', > + allowBlank: false, > + disabled: !me.isCreate, > + onlineValidator: me.isCreate, > + autoSelect: me.isCreate, > + listeners: { > + change: function(f, value) { > + if (me.isCreate) { > + me.loadClusterInterfaces(value, (result) => { > + me.setValues({ network_interfaces: result }); > + }); > + } > + }, > + }, > + listConfig: { > + columns: [ > + { > + header: gettext('Node'), > + dataIndex: 'node', > + sortable: true, > + hideable: false, > + flex: 1, > + }, > + ], > + }, For the node selector, it would be great if existing nodes could be excluded from the dropdown. It isn't possible anyway and throws an error on submit, so excluding them here would be good UX. The `allowedNodes` and/or `disallowedNodes` properties on the selector can be used for this. > + > + }, > + me.node_not_accessible_warning, > + { > + xtype: 'textfield', > + fieldLabel: gettext('Loopback IP'), > + labelWidth: 120, > + name: 'router_id', > + allowBlank: false, > + }, Here, the user-visible name is 'Loopback IP', internally it is named 'Router ID'. In the documentation, 'Router-ID' is used too in the associated documentation. Perhaps just settle on Router-ID? As long as it is explained in the documentation (maybe also add a tooltip?), I wouldn't use a separate name IMHO. > + me.interface_selector, > + ]; > + me.callParent(); > + }, > +}); > + > +Ext.define('PVE.sdn.Fabric.OpenFabric.Node.Edit', { > + extend: 'Proxmox.window.Edit', > + xtype: 'pveSDNFabricAddNode', [..] > + > + initComponent: function() { > + let me = this; > + > + me.node_not_accessible_warning = > Ext.create('Proxmox.form.field.DisplayEdit', { > + userCls: 'pmx-hint', > + value: gettext('The node is not accessible.'), > + hidden: true, > + }); > + > + let ipanel = Ext.create('PVE.sdn.Fabric.OpenFabric.Node.InputPanel', { > + node_not_accessible_warning: me.node_not_accessible_warning, > + isCreate: me.isCreate, > + loadClusterInterfaces: me.loadClusterInterfaces, > + }); > + > + Ext.apply(me, { > + subject: gettext('Node'), Can be moved to a direct member assignment outside of initComponent(). > + items: [ipanel], > + }); > + > + me.callParent(); > + > + if (!me.isCreate) { > + me.loadAllAvailableNodes((allNodes) => { > + if (allNodes.some(i => i.name === me.node)) { > + me.loadClusterInterfaces(me.node, (clusterResult) => { > + me.loadFabricInterfaces(me.fabric, me.node, > (fabricResult) => { > + fabricResult.interface = fabricResult.interface > + .map(i => PVE.Parser.parsePropertyString(i)); > + > + let data = { > + node: fabricResult, > + network_interfaces: clusterResult, > + }; > + > + ipanel.setValues(data); > + }); > + }); nit: perhaps move some of this into functions? This is pretty deeply nested at this point. Would make it definitely more readable. > + } else { > + me.node_not_accessible_warning.setHidden(false); > + // If the node is not currently in the cluster and not > available (we can't get it's interfaces). > + me.loadFabricInterfaces(me.fabric, me.node, (fabricResult) > => { > + fabricResult.interface = fabricResult.interface > + .map(i => PVE.Parser.parsePropertyString(i)); > + > + let data = { > + node: fabricResult, > + }; > + > + ipanel.setValues(data); > + }); > + } > + }); Seems oddly similar to the above, could this be simplified/abstracted a bit over both? If its feasible, of course. > + } > + > + if (me.isCreate) { > + me.method = 'POST'; > + } else { > + me.method = 'PUT'; > + } > + }, > +}); > + > diff --git a/www/manager6/sdn/fabrics/ospf/NodeEdit.js > b/www/manager6/sdn/fabrics/ospf/NodeEdit.js > new file mode 100644 > index 000000000000..d022272b5428 > --- /dev/null > +++ b/www/manager6/sdn/fabrics/ospf/NodeEdit.js > @@ -0,0 +1,207 @@ > +Ext.define('PVE.sdn.Fabric.Ospf.Node.InputPanel', { > + extend: 'Proxmox.panel.InputPanel', [..] > + > +Ext.define('PVE.sdn.Fabric.Ospf.Node.Edit', { Overall, seems this component is mostly the same as the respective OpenFabric counterpart - maybe a common parent ExtJS component can be created? [..] > + > + initComponent: function() { > + let me = this; > + > + me.node_not_accessible_warning = > Ext.create('Proxmox.form.field.DisplayEdit', { > + userCls: 'pmx-hint', > + value: gettext('The node is not accessible.'), > + hidden: true, > + }); > + > + > + let ipanel = Ext.create('PVE.sdn.Fabric.Ospf.Node.InputPanel', { > + interface_selector: me.interface_selector, > + node_not_accessible_warning: me.node_not_accessible_warning, > + isCreate: me.isCreate, > + loadClusterInterfaces: me.loadClusterInterfaces, > + }); > + > + Ext.apply(me, { > + subject: gettext('Node'), > + items: [ipanel], > + }); > + > + me.callParent(); > + > + if (!me.isCreate) { > + me.loadAllAvailableNodes((allNodes) => { > + if (allNodes.some(i => i.name === me.node)) { > + me.loadClusterInterfaces(me.node, (clusterResult) => { > + me.loadFabricInterfaces(me.fabric, me.node, > (fabricResult) => { > + fabricResult.interface = fabricResult.interface > + .map(i => PVE.Parser.parsePropertyString(i)); > + > + let data = { > + node: fabricResult, > + network_interfaces: clusterResult, > + }; > + > + ipanel.setValues(data); > + }); > + }); > + } else { > + me.node_not_accessible_warning.setHidden(false); > + // If the node is not currently in the cluster and not > available (we can't get it's interfaces). > + me.loadFabricInterfaces(me.fabric, me.node, > (fabricResult) => { nit: wrong indentation :^) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel