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.

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hello,

> On 11 Nov 2020, at 11:55, Jonatan Schlag <jonatan.schlag(a)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(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.

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