> Hi,>> >> > Hi Jonatan,> >> > thanks for your feedback. I'm not very familiar with your workflow
> > yet, I hope you will excuse my mistakes.
>> Absolutely. This is just to let you know what was different than
> expected.>> Also feel free to ask any questions whenever you are unsure :)>> > I guess I'll have to submit this patch a second time as well. If
> > you don't mind, could you please comment on my idea for the second
> > attempt:> >> > Patch 1: Only fix HTML output e.g. missing "<", indentation and so
> > on.> > I remember Michael suggested this in the past, but since it only
> > affected a few lines this time, I thought it was okay.
Same here, if there are only some we can overlook it, but i thought it might be could to point these out as well. Splitting things up is always a good idea.>> I do this sometimes, too. A few probably can never be avoided.>> I wouldn’t have complained about this as long as this is in or close
> to the lines that are being changed.
+1>> > I just noticed another issue with the original zoneconf source.
> > Some HTML elements are created with whitespace in their id tag,
> > which isn't allowed by HTML standard.> > Should I fix this in the first patch as well, or would you like
> > this to be another patch?>> Please do this in another patch. That allows us to patch this very
> quickly because it is very easy to review.
What are legacy browsers. Honestly I do not care about everything older then two years, so if these browsers support it, I would say go for it.>> > Patch 2 (or 3?): Add highlighting feature> > This patch would rely on the changes introduced by the first patch.
> > What's the best way to make sure they don't get separated?>> Just send them in one go.>> If you are using “git send-email” it will automatically generate a
> patchset which then looks like this:>>> > I will answer your comments below! I have also added a few specific
> > questions.> >> > Regards> > Leo>> -Michael>> > Am 11.11.2020 um 12:55 schrieb Jonatan Schlag:> > > 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:> > >> > >> > > > 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.> >> > This ensures that the highlighting always takes precendence over
> > the table row colors.> > I found that this was not always reliable during the tests.> >> > I can easily fix this by using modern css selectors like nth-child.
> > This would also simplify the Perl code, because I can do the odd-
> > even row colors in CSS as well.> > But this might break compatibility with legacy browsers. What is
> > your opinion on this?
Please do it as another page. This makes reviewing easier.> > Should this be included the first general clean-up patch?> >
And here I have learned something. So this reviewing is also beneficial to me. 🙂> > > > }> > > >> > > > 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?> > Because it is specific to this highlighting function. I want to
> > avoid cluttering the global namespace with auxiliary functions.
This directory seems reasonable to me. Just at the same licensing header as the Perl file.> > > > +> > > > + //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.> > Where should I put this? Would html/include/zoneconf.js be the
> > right place? Are there any licensing headers I need to add?
So you are free to create such a function, but apparently to Michaels, and we don’t have such a function yet.> > > > @@ -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>> > That would be great! I know this isn't really important, but
> > creating clean HTML makes it much easier for me to check my work.
Thanks.> > > > 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?> > I'll split it!
> > > }> > > }> > > @@ -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