search

TN03 / expandcontract

http://svasti.de/en/?Welcome/Expandcontract
0 stars 1 forks source link

contentPadding kann nur in Pixel angegeben werden #16

Closed cmb69 closed 2 years ago

cmb69 commented 2 years ago

Zur Zeit wird contentPadding nur als Pixelwert richtig behandelt: https://github.com/TN03/expandcontract/blob/c98c475891a8755b6171023e34798348c8f5c513/expandcontract.js#L30

Ich glaube (bin nicht sicher; Überblick verloren), dass hier aber allgemeine CSS-Längen erlaubt sein sollten. Diese gehen aber durch parseInt() verloren, obwohl sie an anderer Stelle dann doch verwendet würden: https://github.com/TN03/expandcontract/blob/c98c475891a8755b6171023e34798348c8f5c513/expandcontract.js#L54

Also entweder nur Pixelwerte akzeptieren, oder irgendwie umrechnen (dürfte nicht ganz trivial sein, aber vielleicht kann man erst das contentPadding setzen, und dann daraus die max-height berechnen; um Flackern zu verhindern, müsste das vielleicht in einem verborgenen Element durchgeführt werden).

frase-git commented 2 years ago

Jetzt weiß ich auch wieder, warum ich in die Tooltipps geschrieben habe: Pixel(!)-Angabe - mit Ausrufezeichen. Bin dafür Padding in px verbindlich vorzuschreiben. Das stellt auch keine wesentliche Behinderung bei der Anwendung dar.

olape-git commented 2 years ago

Aber darf es denn nun nur einfaches xx.px sein, oder geht auch ww.px xx.px yy.px zz.px

frase-git commented 2 years ago

Aber darf es denn nun nur einfaches xx.px sein, oder geht auch ww.px xx.px yy.px zz.px

Es darf definitv nur ein einzelner px-Wert sein oder 0! Alle anderen Varianten (mehrer Werte oder andere Einheiten) führen zu Fehlern bei der Höhenberechnung. Das fällt zwar bei wenig Inhalt kaum auf, bei sehr langem Inhalt wird es aber immer den unteren Teil abschneiden.

olape-git commented 2 years ago

Ist es sinnvoll, auf Werte unter 1, also i.e 0.55px zu prüfen, oder Werte wie 5.555px oder einigen wir uns darauf, nur ganze Pixelwert ab 1 bis 9999?

Wie sieht es im gleichen Zusammnehang bei bei max-height aus?

frase-git commented 2 years ago

content-padding und max-height werden mit parseInt() behandelt und liefern immer nur ganze Zahlen. Es ist nicht zu erwarten, dass Dezimalzahlen (Kommazahlen) eingegeben werden. Negative Zahlen gibt es nicht für padding und max-height, also werden solche Angaben ignoriert. Das merkt man dann schon.

olape-git commented 2 years ago

ok, aber bei max-height sind ja nun auch Angaben in em oder rem möglich. Da ist es ja nun nicht unwahrscheinlich, das Angaben wie 10.5em kommen. Dann sollte der Hilfetext entsprechend angepasst werden. Wenn padding generell nur in px erlaubt ist, ist es dann nicht besser, wir lassen bei der Eingabe bzw. Angabe gleich nur Ziffern zu?

frase-git commented 2 years ago

... aber bei max-height sind ja nun auch Angaben in em oder rem möglich. Da ist es ja nun nicht unwahrscheinlich, das Angaben wie 10.5em kommen.

Wir haben ein Problem: Es gibt max-height für die Berechnung der Content-Höhe (im JS) und max-height als Parameter oder Config-Option. In meiner letzten Antwort habe ich das dummerweise vermischt. Also, du meinst max-height als Parameter - stimmts? Bei diesem Wert ist alles egal. Der wird nur in dieser Zeile als Inline-Style eingefügt ($limitheight). https://github.com/TN03/expandcontract/blob/ec4354e27e6d07fd7b37cefa10e00e9912ec49aa/index.php#L202

Wenn padding generell nur in px erlaubt ist, ist es dann nicht besser, wir lassen bei der Eingabe bzw. Angabe gleich nur Ziffern zu?

Das scheint mir vernünftig. Man müsste das px dann igendwie irgendwo anfügen. Evtl. hier? @TN03 https://github.com/TN03/expandcontract/blob/ec4354e27e6d07fd7b37cefa10e00e9912ec49aa/expandcontract.js#L54 Also vielleicht so el.style.setProperty("padding", contentPadding + "px"); Ich weiß nicht, ob die Eingaben bzw. Parameter dann noch geprüft werden müssten.

cmb69 commented 2 years ago

Nur positive ganze Zahlen als Eingabe zu erlauben, erscheint mir sinnvoll. Andernfalls müssten ggf. negative Paddings noch sonderbehandelt werden (parseInt("-1") === -1).

frase-git commented 2 years ago

Andernfalls müssten ggf. negative Paddings noch sonderbehandelt werden (parseInt("-1") === -1).

Wäre das nicht wieder zu übertrieben? Das macht doch sonst auch keiner. Im Gegensatz zu negativen Margins werden negative Paddings von den Browsern als invalide bewertet und ignoriert. Der Anwender würde das sofort sehen.

Ach übrigens, @cmb69: Happy birthday to you!

olape-git commented 2 years ago

Ach übrigens, @cmb69: Happy birthday to you!

Möchte wissen, warum Frank sowas immer weiß. Ich schließe mich natürlich an.

https://github.com/olape-git/expandcontract/tree/Validate-input-for-max-height%2C-content-padding Damit sollten die CSS Eingaben eigentlich passen bzw unpassende ignoriert werden, auch negative. padding -> nur ganzalig und in px bzw 0 und off height -> ganzzahlig und mit Trennung (Punkt) px, em, rem, %, vh bzw. 0 und off Zusätzlich werden noch Angaben wie EM, REM, PX, VH, OFF ... korrigiert.

Edit Das betrifft sowohl Parameter als auch die Angaben in der config.

Wir sollten die Hilfsfunktionen auslagern --> ecfuncs.php

TN03 commented 2 years ago

Ist das hier

Das scheint mir vernünftig. Man müsste das px dann igendwie irgendwo anfügen. Evtl. hier? @TN03

noch relevant nach dem letzten Verbesserungen https://github.com/TN03/expandcontract/pull/23 ? Oder können wir hier schließen?

TN03 commented 2 years ago

Ach @cmb69: Alles, alles Gute!

frase-git commented 2 years ago

noch relevant nach dem letzten Verbesserungen https://github.com/TN03/expandcontract/pull/23 ? Oder können wir hier schließen?

Es ist noch so, dass man px selbst eintragen muss. Und ich finde: so lassen und schließen.

olape-git commented 2 years ago

Es wird auch nichts anders mehr zugelassen. Fehlt es, wird die Eingabe ignoriert. Von daher alles im grünen.

TN03 commented 2 years ago

Es wird auch nichts anders mehr zugelassen. Fehlt es, wird die Eingabe ignoriert. Von daher alles im grünen.

Ja, genau so hatte ich den Code verstanden.

lck-git commented 2 years ago

Ach @cmb69: Alles, alles Gute!

Auch von mir alles Gute, viel Glück und Gesundheit!

TN03 commented 2 years ago

@lck-git , @frase-git , @olape-git Schaut euch bitte mal bei Gelegenheit diesen Branch an. Es bleibt zwar bei "px", aber jetzt universell; https://github.com/TN03/expandcontract/tree/Content-Padding_more_values

olape-git commented 2 years ago

Die Idee ist gut, die Eingabe muss aber besser geprüft werden. Ich schau morgen mal, ob ich da helfen kann.

TN03 commented 2 years ago

Die Idee ist gut, die Eingabe muss aber besser geprüft werden. Ich schau morgen mal, ob ich da helfen kann.

Das stimmt. Die Prüfung ist zu lax. Ich hab' das schon einmal verbessert: if (!preg_match('/^[0-9]+(px)$/', $padding)) { Siehe auch: https://cmsimpleforum.com/viewtopic.php?f=16&t=18315&start=460#p83526

Das sollte dann eigentlich passen :).

TN03 commented 2 years ago

Closed with https://github.com/TN03/expandcontract/commit/04884edb49c3a0304e1445905c28f4271c7e06e7 and https://github.com/TN03/expandcontract/commit/0da0f68393c35eab12e8e97b0a970c492b40f0c8

olape-git commented 2 years ago

https://github.com/TN03/expandcontract/blob/97b2ba2354ef6f787960656c21c0df923c540f08/index.php#L118

also wenn ich das richtig sehe, dann geht hier alles daneben, was nicht dem Schema xx.px entspricht. Da stehen wir doch wieder am Anfang. Angaben wei 'wwpx xxpx' oder 'wwpx xxpx yypx zzpx' gehen daneben.

cmb69 commented 2 years ago

also wenn ich das richtig sehe, dann geht hier alles daneben, was nicht dem Schema xx.px entspricht.

Aber erst nach dem der String an Leerzeichen gesplittet wurde. Sollte also eigentlich passen. Evtl. könnte man dem Splitten die einzelnen Arrayelemente noch trimmen. Oder vielleicht besser statt explode() preg_split() nutzen, um zu verhindern, dass zusätzliche Whitespace zum Ärgernis werden (e.g. https://3v4l.org/FcnE1).

olape-git commented 2 years ago

Aber erst nach dem der String an Leerzeichen gesplittet wurde. Sollte also eigentlich passen.

Stimmt, da habe ich nicht aufgepasst

olape-git commented 2 years ago

Könnte man noch darüber philosophieren, ob eine Prüfung im Ganzen, vor dem Splitting performanter wäre, als 4x den einzelnen String zu prüfen. Dabei wäre es auch ohne große Umstände möglich für recht und links abweichende Einheiten zuzulassen (falls das in der Praxis überhaupt üblich ist)

olape-git commented 2 years ago

'11px 12px 13px 14px 15px' ist nicht sinnvoll, aber zur Zeit möglich und wird auch so übernommen

TN03 commented 2 years ago

Dabei wäre es auch ohne große Umstände möglich für recht und links abweichende Einheiten zuzulassen (falls das in der Praxis überhaupt üblich ist)

Hier muss zwingend alles in px passieren, weil ansonsten im JS die Höhenberechnung falsch sein wird.

'11px 12px 13px 14px 15px' ist nicht sinnvoll, aber zur Zeit möglich und wird auch so übernommen

Da sehe ich kein Problem. Das verursacht weder einen Laufzeit- noch eine JS - Fehler. Es verhält sich genau so, als wenn der User, wie gewünscht, die stylesheet.css des Templates mit den Styles für das Akkordeon füllt und er das dann so einträgt.

Meiner Meinung nach kann man einem User schon etwas Einarbeitung bzw. RTFM zumuten.

um zu verhindern, dass zusätzliche Whitespace zum Ärgernis werden (e.g. https://3v4l.org/FcnE1).

Das wäre vielleicht noch sinnvoll. Allerdings wird der leere Array-Eintrag sofort als falsch erkannt und der User bekommt sogar eine Meldung, dass seine Angaben Fehler enthalten.

olape-git commented 2 years ago

Hier muss zwingend alles in px passieren, weil ansonsten im JS die Höhenberechnung falsch sein wird.

Aber eigentlich doch nur für oben und unten, oder? Also nur als Frage. Ich finde die Möglichkeit in px vollkommen ausreichend. Aber es wird sich immer jemand finden ...

Da sehe ich kein Problem. Das verursacht weder einen Laufzeit- noch eine JS - Fehler. Es verhält sich genau so, als wenn der User, wie gewünscht, die stylesheet.css des Templates mit den Styles für das Akkordeon füllt und er das dann so einträgt.

Ja, es steht dann eben einfach so falsch drin. Probleme macht es keine.

Meiner Meinung nach kann man einem User schon etwas Einarbeitung bzw. RTFM zumuten.

Das ist eigentlich auch immer meine Meinung. Wofür ich übrigens sehr oft kritisiert werde. Zu den beiden letzten muss ich an dieser Stelle aber sagen, wir haben hier schon so viel versucht abzufangen, dass ich es nur konsequent finde, das auch hier noch zu tun.

Das wäre vielleicht noch sinnvoll. Allerdings wird der leere Array-Eintrag sofort als falsch erkannt und der User bekommt sogar eine Meldung, dass seine Angaben Fehler enthalten

Ich glaube trotzdem, es wäre besser. Denn ein oder zwei Leerzeichen ist wirklich nicht immer unbedingt auf den ersten Blick zu erkennen. Auch falls es denn tatsächlich noch die Möglichkeit gibt, dass nicht alle Editoren es so händeln wie Tiny 4.

TN03 commented 2 years ago

Alles gut :). Ich finde diese ganze Validierung hier schon sehr grenzwertig. Aber wir haben's halt übertrieben, dann sollten wir auch konsequent bleiben. Könntest Du deinen Master für Tests vielleicht mal im Board ankündigen? Es muss ja lediglich index.php getauscht werden. Und da hast du ja auch schon alle anderen Fehlerbereinigengen (on/off - die Tücke der RegExp ;-) ) mit drin? Ich kann jetzt leider erst wieder nur abends / nachts und nur begrenzt.

Auch falls es denn tatsächlich noch die Möglichkeit gibt, dass nicht alle Editoren es so händeln wie Tiny 4.

Das verstehe ich noch immer nicht. Kannst du erklären wo das Problem ist? Es ist doch egal, wie der Editor das macht. Es werden halt alle Leerzeichen vor und nach dem String entfernt - multibyte-fähig war der Punkt.

olape-git commented 2 years ago

Das verstehe ich noch immer nicht. Kannst du erklären wo das Problem ist? Es ist doch egal, wie der Editor das macht. Es werden halt alle Leerzeichen vor und nach dem String entfernt - multibyte-fähig war der Punkt.

Genau, vor und nach. Aber nicht mittendrin. (Es geht ja hier nur um padding und falsche Eingaben mit mehr als einem Leerzeichen Trennung. Das zu korrigieren und nicht mit einer Fehlermeldung abzufangen.)

Und so lange das immer so ist, wie aktuell bei Tiny, dass es jeweils min. eins, aber auch nur ein "richtiges" Leerzeichen ist und der Rest irgendwas anderes (was auch immer das ist), dann wird "explode auf Leezeichen" auch funktionieren. Aber ist es immer so?

Also alles kein muss für ein korrekte Funktion. Aber Usereingaben wenigstens per Trim auf zu bererinigen ist eigentlich gängige Praxis. Bisher habe ich das auch immer für ausreichend gehalten. Bis du die Leerzeichen von Tiny ins Spiel gebracht hast. Das hatte ich noch nicht auf dem Schirm.

Mit der Option in der config kann man das ausprobieren. Wer dort mal ein 4-wertiges Padding eingibt (wirklich per Hand eingibt), mit wenigstens an einer Stelle mehr als einem Leerzeichen Trennung, der wird festellen es funktioniert nicht mehr. Korrekt, wie ich betonen möchte. Aber auch hier wieder, wir haben hier so viel abgefangen, das ich meien es auch hier noch zu Ende bringen zu können. Macht man das in Tiny funktioniert es, wei ltiny eben egal wie viel Leerzeichen hintereinander kommen, nur ein 'normales Leerzeichen schreibt'

cmb69 commented 2 years ago

Also statt https://github.com/TN03/expandcontract/blob/97b2ba2354ef6f787960656c21c0df923c540f08/index.php#L110

eben

$t = preg_split(`/\s+/u`, $temp);

Vgl. https://3v4l.org/V1PM6