Re: [pve-devel] [PATCH widget-toolkit v5 4/7] fix #3892: Network: add bridge vids field for bridge_vids

[Aaron Lauterer <a.lauterer@proxmox.com>]

On  2024-11-11  21:55, Thomas Lamprecht wrote:
> Am 02.10.24 um 15:11 schrieb Aaron Lauterer:
>> The new optional bridge_vids field allows to set that property via the
>> GUI. Since the backend needs to support it, the field needs to be
>> explicitly enabled.
>>
>> For now, Proxmox VE (PVE) is the use case.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> Tested-By: Stefan Hanreich <s.hanreich@proxmox.com>
>> Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>
>> ---
>> changes since
>> v4: none
>> v3:
>> * switched regex to one with non-capturing group
>> * reworked valid VLAN check according to the suggestion
>> v2:
>> * added validation code following how it is implemented in the API
>>
>>   src/node/NetworkEdit.js | 65 +++++++++++++++++++++++++++++++++++++++++
>>   src/node/NetworkView.js |  5 ++++
>>   2 files changed, 70 insertions(+)
>>
>> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
>> index 27c1baf..bfd0268 100644
>> --- a/src/node/NetworkEdit.js
>> +++ b/src/node/NetworkEdit.js
>> @@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
>>       extend: 'Proxmox.window.Edit',
>>       alias: ['widget.proxmoxNodeNetworkEdit'],
>>   
>> +    // Enable to show the VLAN ID field
>> +    bridge_set_vids: false,
>> +
>>       initComponent: function() {
>>   	let me = this;
>>   
>> @@ -57,11 +60,70 @@ Ext.define('Proxmox.node.NetworkEdit', {
>>   	}
>>   
>>   	if (me.iftype === 'bridge') {
>> +	    let vids = Ext.create('Ext.form.field.Text', {
>> +		fieldLabel: gettext('Bridge VIDS'),
> 
> I know ifupdown2 names it VIDS, but is that really a good name here?
> AFAICT it's not used outside of ifupdown2/cumuls, or do you got any references?
> 
> Maybe "Bridge VLAN IDs" would be a bit more telling?

I tested it, and that is long enough to cause a line break in the label. 
I think I would opt for "VLAN IDs". It is only present on linux bridges 
and with the info box below as well it should be clear what it is for, 
without causing layout issues.

Will send a small v6 with the other suggestions too.




_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel