From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer 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 16:01:31 +0000 Message-ID: <3C91C15A-19A6-4867-9481-58C1F3B0AB0A@ipfire.org> In-Reply-To: <4daf0bf1db662be1a68a896ee8378cf8fb04b88b.camel@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4266343214656903387==" List-Id: --===============4266343214656903387== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 11 Nov 2020, at 11:55, Jonatan Schlag wrot= e: >=20 > Hi, >=20 > thirst thank you for working on this so fast. I will comment on this > patch inline, but first one note on the commit message. >=20 > 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. >=20 > For further information on a good commit message see:=20 > https://blog.ipfire.org/post/how-to-write-a-good-commit-message Good pointers. > "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 >>=20 >> Signed-off-by: Leo-Andres Hofmann >> --- >> html/cgi-bin/zoneconf.cgi | 89 +++++++++++++++++++++++++++++--------- >> - >> 1 file changed, 67 insertions(+), 22 deletions(-) >>=20 >> 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"; >>=20 >> -my $css =3D <> +my $extraHead =3D <> >> + >> + >> END >> ; >=20 > 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. 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. >>=20 >> @@ -162,7 +200,7 @@ foreach (@nics) { >> } >> } >>=20 >> -&Header::openpage($Lang::tr{"zoneconf title"}, 1, $css); >> +&Header::openpage($Lang::tr{"zoneconf title"}, 1, $extraHead); >> &Header::openbigbox('100%', 'center'); >>=20 >> ### Evaluate POST parameters ### >> @@ -312,8 +350,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) { >> print <>
>> >> - >> - >> + >> END >> ; >>=20 >> @@ -332,7 +370,7 @@ foreach (@zones) { >> my $red_restricted =3D ($uc eq "RED" && ! ($red_type eq "STATIC" || >> $red_type eq "DHCP")); >>=20 >> 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 *Michael The web UI is built like this now. I understand your idea, but I would be aga= inst a major rewrite of this now, because it is a lot of work for almost no b= enefit to the user. Since we usually change the CGI files not very much, I gu= ess we can rather cope with a little bit extra overhead here. >>=20 >> next; # We're done here >> } >> @@ -350,7 +388,7 @@ foreach (@zones) { >> } >>=20 >> print <> - "; >> +print "\t\n"; >>=20 >> my $slightlygrey =3D ""; >>=20 >> @@ -370,12 +408,13 @@ foreach (@nics) { >> my $nic =3D $_->[1]; >> my $wlan =3D $_->[2]; >>=20 >> - print ""; >> + print "\t\n\t\t\n"; >>=20 >> # 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 ""; >>=20 >> if ($dev_name eq "") { # Again, skip the zone if it is not activated >> next; >> @@ -391,9 +430,10 @@ foreach (@nics) { >>=20 >> if ($mac eq $ethsettings{"${uc}_MACADDR"}) { >> $checked =3D "checked"; >> + $highlight =3D $_; >> } >>=20 >> - print ""; >> + print "\t\t\n"; >> next; # We're done here >=20 > This is very hard to read, could we split this up please? >> } >> } >> @@ -430,21 +470,26 @@ foreach (@nics) { >>=20 >> $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 $_; >> + } >>=20 >> print <> - >> + >> END >> ; >> } > Same here, when you only change the indention it is hard to see the > actual change of the javascript.=20 >=20 > 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 >>=20 > - print ""; > + print "\t\n"; >=20 > if ($slightlygrey) { > $slightlygrey =3D ""; > -- > 2.27.0.windows.1 >=20 >=20 Thank you for Jonatan for reviewing this :) And thank you even more for Leo-Andres for working on this. Best, -Michael --===============4266343214656903387==--
>> +
$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> >> +