From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Hofmann To: development@lists.ipfire.org Subject: 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. Date: Wed, 11 Nov 2020 17:44:50 +0100 Message-ID: <88af4ce2-b824-37d8-bff0-1dc54c721ed7@leo-andres.de> In-Reply-To: <4daf0bf1db662be1a68a896ee8378cf8fb04b88b.camel@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6093788640970827719==" List-Id: --===============6093788640970827719== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Jonatan, thanks for your feedback. I'm not very familiar with your workflow yet, I hop= e you will excuse my mistakes. I guess I'll have to submit this patch a second time as well. If you don't mi= nd, 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 f= ew lines this time, I thought it was okay. I just noticed another issue with the original zoneconf source. Some HTML ele= ments are created with whitespace in their id tag, which isn't allowed by HTM= L standard. Should I fix this in the first patch as well, or would you like this to be an= other patch? Patch 2 (or 3?): Add highlighting feature This patch would rely on the changes introduced by the first patch. What's th= e best way to make sure they don't get separated? I will answer your comments below! I have also added a few specific questions. Regards Leo 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: > https://blog.ipfire.org/post/how-to-write-a-good-commit-message > > > "Leo-Andres Hofmann" hofmann(a)leo-andres.de =E2=80=93 10. November 2020 13= :37 >> Discussion: lists.ipfire.org/pipermail/development/2020- >> October/008567.html >> >> Signed-off-by: Leo-Andres Hofmann >> --- >> 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 =3D <> +my $extraHead =3D <> >> + >> + >> END >> ; > Could you put this in a separate file (zoneconf.js) please? It's hard to re= ad here and we should try to at least avoid introducing new mixing of languag= es (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 a= nd therefore not part of the patch. Where should I put this? Would html/include/zoneconf.js be the right place? A= re there any licensing headers I need to add? >> @@ -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 >> ; >> >> @@ -332,7 +370,7 @@ foreach (@zones) { >> my $red_restricted =3D ($uc eq "RED" && ! ($red_type eq "STATIC" || >> $red_type eq "DHCP")); >> >> if ($red_restricted) { >> - print ""; >> + print "\t\t\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 That would be great! I know this isn't really important, but creating clean H= TML makes it much easier for me to check my work. >> next; # We're done here >> } >> @@ -350,7 +388,7 @@ foreach (@zones) { >> } >> >> print <> - "; >> +print "\t\n"; >> >> my $slightlygrey =3D ""; >> >> @@ -370,12 +408,13 @@ foreach (@nics) { >> my $nic =3D $_->[1]; >> my $wlan =3D $_->[2]; >> >> - print ""; >> + print "\t\n\t\t\n"; >> >> # Iterate through all zones and check if the current NIC is assigned >> to it >> foreach (@zones) { >> my $uc =3D uc $_; >> my $dev_name =3D $ethsettings{"${uc}_DEV"}; >> + my $highlight =3D ""; >> >> 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 =3D "checked"; >> + $highlight =3D $_; >> } >> >> - print ""; >> + print "\t\t\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"} =3D ($access_selected{"NATIVE"} eq "") && > ($access_selected{"VLAN"} eq "") ? "selected" : ""; > my $vlan_disabled =3D ($wlan) ? "disabled" : ""; > + > + # If the interface is assigned, hightlight table cell > + if ($access_selected{"NONE"} eq "") { > + $highlight =3D $_; > + } > > print < - > + > 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 so= metimes 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 ""; > + print "\t\n"; > > if ($slightlygrey) { > $slightlygrey =3D ""; > -- > 2.27.0.windows.1 > > --===============6093788640970827719==--
>> +
$uc ($red_type)$uc ($red_type)$uc
>> + 		$uc
> 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". >>
$nic
$mac
> textcenter'>$nic
$mac		> id=3D'PPPACCESS $mac' name=3D'PPPACCESS' value=3D'$mac' $checked>> type=3D'radio' id=3D'PPPACCESS $mac' name=3D'PPPACCESS' value=3D'$mac' dat= a- >> zone=3D'RED' data-mac=3D'$mac' onchange=3D'highlightAccess(this)' >> $checked> > - > - $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" $field_disabled> > - > + > + $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" $field_disabled> > +