New subject: [PATCH] zoneconf.cgi: Improve the usability of the zone configuration by marking assigned NICs in the zone color. The highlighting is initially applied to the static HTML output, and JavaScript is used to dynamically follow changes made by the user.

11 Nov 2020

Hi,
thirst thank you for working on this so fast. I will comment on this
patch inline, but first one note on the commit message.
The commit message should be split up. When you use
'git commit -sv' an editor will open and in this editor, the first line
makes up the subject of the mail the lines after the second makes up
the commit message. The first line should not be longer than 50
characters to avoid, a subject like in this E-Mail.
For further information on a good commit message see: 
https://blog.ipfire.org/post/how-to-write-a-good-commit-message
"Leo-Andres Hofmann" hofmann@leo-andres.de – 10. November 2020 13:37
...
Discussion: lists.ipfire.org/pipermail/development/2020-
October/008567.html
Signed-off-by: Leo-Andres Hofmann hofmann@leo-andres.de
html/cgi-bin/zoneconf.cgi | 89 +++++++++++++++++++++++++++++---------



1 file changed, 67 insertions(+), 22 deletions(-)

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index d99a3e611..2501901c4 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -26,7 +26,7 @@ require '/var/ipfire/general-functions.pl';
require "${General::swroot}/lang.pl";
require "${General::swroot}/header.pl";
-my $css = <<END
+my $extraHead = <<END
<style>
table {
width: 100%;
@@ -64,19 +64,19 @@ my $css = <<END
}

td.green {
- background-color: $Header::colourgreen;
+ background-color: $Header::colourgreen !important;
Why do we need !important here? When it does not work without we
should, investigate why. Using important break the specificity of the
CSS, which will make the CSS harder to maintain.
...
}
td.red {

background-color: $Header::colourred;


background-color: $Header::colourred !important;

}
td.blue {

background-color: $Header::colourblue;


background-color: $Header::colourblue !important;

}
td.orange {

background-color: $Header::colourorange;


background-color: $Header::colourorange !important;

}
td.topleft {
@@ -112,6 +112,44 @@ my $css = <<END
margin-top: 1em;
}
</style>



+<script type="text/javascript">

//highlight interface access selection, call this from "onchange"

event of selection element

function highlightAccess(selectObj) {
if(!(selectObj && ('zone' in selectObj.dataset) && ('mac' in

selectObj.dataset))) {

return; //required parameters are missing
}

var zoneColor = selectObj.dataset.zone.trim().toLowerCase(); //zone

color (red/green/blue/orange) CSS class


function colorParentCell(obj, color, enabled = true) { //find

nearest parent table cell of "obj" and set its CSS color class

do {
obj = obj.parentElement;
} while(obj && (obj.nodeName.toUpperCase() !== 'TD'));
if(obj && (['green', 'red', 'orange', 'blue'].includes(color))) {
obj.classList.toggle(color, enabled);
}
}

Why is this function part of the other function?
...


//handle other associated input fields
if(selectObj.type.toUpperCase() === 'RADIO') { //this is a radio

button group: clear all highlights

let radios = document.getElementsByName(selectObj.name);
radios.forEach(function(button) {
if(button.nodeName.toUpperCase() === 'INPUT') { //make sure the

found element is a button

colorParentCell(button, zoneColor, false); //remove css
}
});
} else { //this is a dropdown menu: enable/disable additional VLAN

tag input box

let tagInput = document.getElementById('TAG ' +

selectObj.dataset.zone + ' ' + selectObj.dataset.mac); //tag input
element selector

if(tagInput) {
tagInput.disabled = (selectObj.value !== 'VLAN'); //enable tag

input if VLAN mode selected

}
}

//if interface is assigned, highlight table cell in zone color
colorParentCell(selectObj, zoneColor, (selectObj.value !==

'NONE'));

}

+</script>
END
;
Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.
...
@@ -162,7 +200,7 @@ foreach (@nics) {
}
}
-&Header::openpage($Lang::tr{"zoneconf title"}, 1, $css);
+&Header::openpage($Lang::tr{"zoneconf title"}, 1, $extraHead);
&Header::openbigbox('100%', 'center');
### Evaluate POST parameters ###
@@ -312,8 +350,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
print <<END
<form method='post' enctype='multipart/form-data'>
<table>
- <tr>
- <td class="h narrow topleft" /td>
+ <tr>
+ <td class="h narrow topleft"></td>
END
;
@@ -332,7 +370,7 @@ foreach (@zones) {
my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" ||
$red_type eq "DHCP"));
if ($red_restricted) {

print "<td class='h textcenter $_'>$uc ($red_type)</td>";


print "\t\t<td class='h textcenter $_'>$uc ($red_type)</td>\n";

@Micheal or somebody else, do we have a function to print lines
intended like in the network code of ipfire 3.x?
Something like print_indented <number of tabs> <string>
...
next; # We're done here
}
@@ -350,7 +388,7 @@ foreach (@zones) {
}
print <<END

<td class='h textcenter $_'>$uc</br>



<td class='h textcenter $_'>$uc<br>


Should be not part of this patch, but does not matter much. You can
use 'git add -i' to pick those changes and but them in a separate patch
like "cleaning up code".
...
<select name="MODE $uc">
<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf
nicmode default"}</option>
<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf
nicmode bridge"}</option>
@@ -361,7 +399,7 @@ END
;
}
-print "</tr>";
+print "\t</tr>\n";
my $slightlygrey = "";
@@ -370,12 +408,13 @@ foreach (@nics) {
my $nic = $_->[1];
my $wlan = $_->[2];

print "<tr><td class='h narrow textcenter'>$nic<br>$mac</td>";


print "\t<tr>\n\t\t<td class='h narrow

textcenter'>$nic<br>$mac</td>\n";
# Iterate through all zones and check if the current NIC is assigned
to it
foreach (@zones) {
my $uc = uc $_;
my $dev_name = $ethsettings{"${uc}_DEV"};

my $highlight = "";

if ($dev_name eq "") { # Again, skip the zone if it is not activated
next;
@@ -391,9 +430,10 @@ foreach (@nics) {
if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
$checked = "checked";

$highlight = $_;

}

print "<td class='textcenter $slightlygrey'><input type='radio'

id='PPPACCESS $mac' name='PPPACCESS' value='$mac' $checked></td>";

print "\t\t<td class='textcenter $highlight $slightlygrey'><input

type='radio' id='PPPACCESS $mac' name='PPPACCESS' value='$mac' data-
zone='RED' data-mac='$mac' onchange='highlightAccess(this)'
$checked></td>\n";
next; # We're done here
This is very hard to read, could we split this up please?
...
}
}
@@ -430,21 +470,26 @@ foreach (@nics) {
$access_selected{"NONE"} = ($access_selected{"NATIVE"} eq "") &&
($access_selected{"VLAN"} eq "") ? "selected" : "";
my $vlan_disabled = ($wlan) ? "disabled" : "";


# If the interface is assigned, hightlight table cell
if ($access_selected{"NONE"} eq "") {
$highlight = $_;
}

print <<END

<td class="textcenter $slightlygrey">

<select name="ACCESS $uc $mac"

onchange="document.getElementById('TAG $uc $mac').disabled =
(this.value === 'VLAN' ? false : true)">

<option value="NONE" $access_selected{"NONE"}>- $Lang::tr{"zoneconf


access none"} -</option>

<option value="NATIVE"


$access_selected{"NATIVE"}>$Lang::tr{"zoneconf access
native"}</option>

<option value="VLAN" $access_selected{"VLAN"}


$vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>

</select>

<input type="number" class="vlanid" id="TAG $uc $mac" name="TAG $uc

$mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>

</td>



<td class="textcenter $highlight $slightlygrey">

<select name="ACCESS $uc $mac" data-zone="$uc" data-mac="$mac"

onchange="highlightAccess(this)">

<option value="NONE" $access_selected{"NONE"}>- $Lang::tr{"zoneconf


access none"} -</option>

<option value="NATIVE"


$access_selected{"NATIVE"}>$Lang::tr{"zoneconf access
native"}</option>

<option value="VLAN" $access_selected{"VLAN"}


$vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>

</select>

<input type="number" class="vlanid" id="TAG $uc $mac" name="TAG $uc

$mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>

</td>


END
;
}
Same here, when you only change the indention it is hard to see the
actual change of the javascript.
To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
Greetings Jonatan
...
- print "</tr>";
+ print "\t</tr>\n";
if ($slightlygrey) {
$slightlygrey = "";
--
2.27.0.windows.1

    

Re: [PATCH] zoneconf.cgi: Improve the usability of the zone configuration by marking assigned NICs in the zone color. The highlighting is initially applied to the static HTML output, and JavaScript is used to dynamically follow changes made by the user.

Signed-off-by: Leo-Andres Hofmann hofmann@leo-andres.de

Signed-off-by: Leo-Andres Hofmann hofmann@leo-andres.de

Signed-off-by: Leo-Andres Hofmann hofmann@leo-andres.de

Signed-off-by: Leo-Andres Hofmann hofmann@leo-andres.de