Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: revert PJAX to 5.11 behaviour #4337

Merged
merged 15 commits into from Jan 24, 2021
Merged

UI: revert PJAX to 5.11 behaviour #4337

merged 15 commits into from Jan 24, 2021

Conversation

schuer
Copy link
Member

@schuer schuer commented Jan 17, 2021

TODO:

  • PJAX-Selektoren und -Switches reduzieren, so dass nur noch der Main-Content beachtet wird
  • PJAX-Container entfernen (Navigation, Header)
  • page-Parameter in URL vergleichen und PJAX nicht nutzen, wenn eine andere Seite aus dem Contentbereich heraus geladen wird
  • Download-Links beachten (Download-Links ohne PJAX #4335)

fixes #4335

@schuer schuer added this to the REDAXO 5.12 milestone Jan 17, 2021
@schuer schuer marked this pull request as ready for review January 23, 2021 14:33

// handle redaxo links to pages other than current page
// hint: check global `rex.page` variable
// hint: use custom `data-pjax-cancel-request` since `data-pjax-state` would be overwritten
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wann soll man jetzt data-pjax-cancel-request notieren? Und warum?

data-pjax=false haben wir ja auch noch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data-pjax-cancel-request wird nur von uns intern benutzt, wenn wir nach Prüfung feststellen, dass ein Element nicht das dynamische Laden von PJAX auslösen soll. Habe es gerade im nachfolgenden Kommentar ausführlich beschrieben.

Ein eigenes Data-Attribut deshalb, weil PJAX sein data-pjax-state eigenständig überschreibt, ohne dass wir es vorher auswerten konnten.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Achso, und du hast natürlich recht, @staabm, wir nutzen bereits das data-pjax-Attribut. Das wird aber initial als Selector verwendet und nur auf false geprüft. Ich habe dieses neue data-pjax-cancel-request eingeführt, weil ich hinterlegen wollte, was der Grund fürs Cancel ist, falls man zukünftig darauf noch weiter eingehen wollte.

@schuer
Copy link
Member Author

schuer commented Jan 23, 2021

Okay, ich bin mit den Anpassungen durch und möchte aber anmerken, dass ich nur einige Situationen getestet habe, die REDAXO mir standardmäßig mitbringt. Also die typischen Core-AddOns, das Speichern von Formularen, Editieren von Inhalten, usw.
Es muss definitiv nochmal ausführlich und im Detail getestet werden, besonders auch mit prominenten AddOns!


Ich bin über eine Sache bei PJAX etwas unglücklich und hadere sogar im Stillen, ob wir den Switch wirklich vorgenehmen wollen, oder ob wir vorerst doch lieber wieder auf die alte jQuery-PJAX-Lib zurückwechseln wollen:

Die neue Lib funktioniert so, dass es die angegebenen Selektoren verwendet werden, um mittels querySelectorAll <a>- und <form>-Elemente zu finden, die dynamisch Inhalte laden sollen. Dazu bekommen diese ein data-pjax-state-Attribut verpasst und EventHandler aufgebunden, die bei click/Enter das Laden anstoßen. Der Lade-Prozess funktioniert so, dass PJAX zunächst ein paar Prüfungen vornimmt, um manche Elemente auszuschließen, etwa Links, die auf externe URLs verweisen, die Hashes in der URL enthalten oder die bereits ein data-pjax-state-Attribut besitzen.
Und dann kommt der Punkt, der mir nicht gefällt: Es wird ein event.preventDefault() abgesetzt (Source), bevor die Ladeprozedur startet.

Unser Problem ist, dass wir bei Klick auf unsere Links gerne noch selbst prüfen möchten, ob das Ziel dynamisch geladen werden soll. Beim Wechsel der Seiten (?page=) möchten wir beispielsweise nicht mittels PJAX laden, sondern auf normalem Weg. Nur leider hat PJAX an der Stelle, an der wir noch sinnvoll einhaken können (loadUrl), bereits das preventDefault() abgesetzt. Wir können dann nur noch manuell die Zielseite laden, hätten uns aber eigentlich gewünscht, dass PJAX dieses Element einfach ignoriert und möglichen anderen Events die Chance lässt, ihre Dinge zu tun. Etwa Tabs, die Inhalte wechseln wollen.

tl;dr: Es fehlt also eine sinnvolle Möglichkeit, nach dem Klick auf ein PJAX-Element den Ladeprozess zu umgehen.

Wenn sich nun nach Tests zeigt, dass alles funktioniert wie es soll: Okay, dann könnte man die neue PJAX-Lib verwenden. Ich habe nur etwas Sorge, dass manche AddOns oder deren Nutzer mit Event-Konstrukten arbeiten, die nicht mehr erreicht werden, weil zuvor entweder PJAX eingegriffen hat oder wir bereits einen manuellen Ladevorgang angestoßen haben.

@schuer schuer requested review from gharlan and staabm January 23, 2021 15:38
@gharlan
Copy link
Member

gharlan commented Jan 24, 2021

Also ich habe mir das technische Problem noch nicht weiter angeschaut. Erstmal nur so allgemein:

Einerseits fände ich es natürlich sehr schade, wenn wir deine ganze Arbeit wieder rückgängig machen würden. Vor allem, weil es ja eine von allen gewollte Richtung ist (modernere PJAX-Lib, ohne jQuery).
Andererseits ist natürlich die Frage, insbesondere wo wir das seitenübergreifende PJAX-Loading wieder rausnehmen, welche Vorteile wir von dem Schritt real haben. jQuery ganz entfernen werden wir frühstens mit R6 können. Somit, da der pjax-Wechsel nicht ganz unproblematisch ist, könnte man ihn ebenso auf R6 verschieben.
Nochmal andererseits wäre es aber schon mal ein erledigter Schritt in die richtige Richtung.

Bin mir unsicher.

@staabm
Copy link
Member

staabm commented Jan 24, 2021

Ich denke wir sollten eine längere beta starten und auf feedback von den testern warten, um zu sehen wie problematisch es ist

@schuer
Copy link
Member Author

schuer commented Jan 24, 2021

Danke für euer Feedback! Ich habe etwas Sorge, dass wir im Main-Branch nun mit der neuen PJAX-Lib weiter laufen und womöglich beim Testen mit Nutzern bemerken, dass es in manchen Situationen klemmt. Dann wären wir im Zugzwang, die Probleme fixen zu müssen, und müssten dafür womöglich die Lib anpassen.

Die Lib ist zwar neuer und moderner und vor allem ohne jQuery, allerdings wird sie auch bereits nicht mehr wirklich gepflegt. Alle Maintainer kümmern sich inzwischen um andere Themen und haben kein offensichtliches Interesse mehr an PJAX.

Aufgrund der im letzten Kommentar genannten technischen Dinge habe ich das Gefühl, dass wir selbst eingreifen müssten, um PJAX für REDAXO tauglich zu machen. Es fehlt uns wie beschrieben die Möglichkeit, nach Prüfung den Ladeprozess abzubrechen. Und eben habe ich bemerkt, dass auch der Umgang mit Ankern/Hashes in der URL ungünstig ist: Speichert man in REDAXO einen Slice ein zweites Mal, hakt PJAX aufgrund des Hashes aus, so dass das Formular klassisch abgeschickt wird und die Seite manuell lädt.

Mein Bauchgefühl ist deshalb:

  • Ich würde für die kommende 5.12 lieber wieder auf das alte jQuery-PJAX zurückbauen wollen
  • Das neue PJAX benötigt zunächst Anpassungen, die wir als PR einbringen könnten, bevor es sich ohne Workarounds für REDAXO eignet.

Mein Vorschlag wäre deshalb, diesen PR zunächst zu mergen und anschließend den Stand in einen neuen WIP-Branch auszulagern. Einen neuen PR aufsetzen, um aufs alte jQuery-PJAX zurückzuwechseln.

@gharlan
Copy link
Member

gharlan commented Jan 24, 2021

Ich habe etwas Sorge, dass wir im Main-Branch nun mit der neuen PJAX-Lib weiter laufen und womöglich beim Testen mit Nutzern bemerken, dass es in manchen Situationen klemmt. Dann wären wir im Zugzwang, die Probleme fixen zu müssen, und müssten dafür womöglich die Lib anpassen.

Die Sorge ist schon nicht ganz unberechtigt, denke ich.
Die Erfahrung sagt mir, egal wie lange man die Beta-Phase macht, oftmals zeigen sich Probleme im Zusammenspiel mit anderen Addons erst nach dem Release, wenn die große Masse updatet.
Und wenn wir dann erstmal mit der neuen pjax-Lib released haben, haben wir dann eventuell ein Problem.

Die Lib ist zwar neuer und moderner und vor allem ohne jQuery, allerdings wird sie auch bereits nicht mehr wirklich gepflegt. Alle Maintainer kümmern sich inzwischen um andere Themen und haben kein offensichtliches Interesse mehr an PJAX.

Das ist auch etwas, was mich bisschen stört. Also dass wir zu einer Lib wechseln, die auch nicht so richtig mehr weiterentwickelt wird. Bloß halt nicht schon ganz so lange, wie unsere bisherige.

Also ich glaube, ich würde daher Dirks Vorschlag folgen. Auch wenn ich mir weiter unsicher bin.

@skerbis
Copy link
Contributor

skerbis commented Jan 24, 2021

Dem stimme ich zu. Wir sollten das doch lieber zu einem späteren Zeitpunkt angehen. Ich schmeiß dann noch gleich ne Alternative in den Ring https://htmx.org/

@gharlan gharlan added the automerge Automatisch PR rebasen und mergen label Jan 24, 2021
@kodiakhq kodiakhq bot merged commit 2aecb71 into master Jan 24, 2021
@kodiakhq kodiakhq bot deleted the schuer-pjax branch January 24, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatisch PR rebasen und mergen
Development

Successfully merging this pull request may close these issues.

Download-Links ohne PJAX
4 participants