Hello,
On 11 Nov 2020, at 11:55, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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
Good pointers.
"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.
Hmm, I get that the JS is messy. But I am not sure if a second file helps.
I would say since this is an extension of an existing file, we should keep it as it is.
@@ -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>
*Michael
The web UI is built like this now. I understand your idea, but I would be against a major rewrite of this now, because it is a lot of work for almost no benefit to the user. Since we usually change the CGI files not very much, I guess we can rather cope with a little bit extra overhead here.
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
Thank you for Jonatan for reviewing this :)
And thank you even more for Leo-Andres for working on this.
Best, -Michael