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.

[Jonatan Schlag <jonatan.schlag@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 9373 bytes --]

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(a)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(a)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