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

markdown: setBreaksEnabled( ) einstellbar machen #3885

Merged
merged 12 commits into from Oct 2, 2020
Merged

markdown: setBreaksEnabled( ) einstellbar machen #3885

merged 12 commits into from Oct 2, 2020

Conversation

christophboecker
Copy link
Contributor

Der Markdown-Parser fasst im Normalbetrieb aufeinanderfolgende Zeilen zu einem Block zusammen und setzt sie in einen P-Tag. Falls das nicht gewünscht ist. kann mit $parser->setBreaksEnabled(true) das Verhalten geändert werden. Dann wird jeder Zeilenumbruch im Quelltext zu einem <br>. Und so ist aktuell Markdown in REX eingestellt. Und das kann bisher an keiner Stelle rückgängig gemacht werden.

Für die Texterfassung bedeutet es, dass man in der Ausgabe mehrzeiligen Fließtext bei der Erfassung immer als eine Zeile schreiben muss, um auch bei verschiedenen Anzeigebreiten einen guten Umbruch zu erhalten. Manche empfinden das als unübersichtlich.

Erfasst man den Text anders, kann es jedoch seltsam werden ... Hier mal ein Beispiel; das Zeilenende des erfassten Textes ist mit kenntlich gemacht:
grafik

Ausgabe mit der Einstellung TRUE-> kein Fließtext
grafik

Ausgabe mit der Einstellung TRUE in einem schmalen DIV -> nur Fließtext für die dritte Zeile
grafik

Ausgabe mit der Einstellung FALSE als Fließtext:
grafik

Der PR hier würde den fixen Wert TRUE durch den aktuellen Wert der Property 'rex_markdown_setBreaksEnabled' ersetzen, wobei zur Vermeidung eines BC TRUE der Defaultwert bei nicht gesetzter Property ist. Mit

rex::setProperty('rex_markdown_setBreaksEnabled',false);

kann der REDAXO-Default true überschrieben werden.

Statt setBreaksEnabled fix auf TRUE einzustellen, kann man nun über `rex::setProperty('rex_markdown_setBreaksEnabled',false)` den Default-Wert (true) überschreiben.
@gharlan
Copy link
Member

gharlan commented Sep 25, 2020

Einen globalen Schalter dafür einzuführen, finde ich nicht sinnvoll. Dann kann man sich als Addon-Ersteller ja nicht mehr darauf verlassen, wie die Markdown-Dateien des Addons bei anderen gerendert werden.

Ich hatte die Option damals aktiviert, weil ich ein möglichst gleiches Rendering haben wollte wie Markdown hier auf Github.
Dabei unterlag ich aber einem Irrtum. Denn das Verhalten auf GitHub ist nicht überall gleich. Hier in den Markdown-Kommentaren ist das Verhalten so wie in Redaxo, aber in Markdown-Dateien auf Github (Readmes etc.) hingegen nicht.

Da es in REDAXO aber gerade ja um die Readmes geht, habe ich das Gegenteil meiner Intention erreicht.
Daher, und auch weil die von dir beschriebene Problematik mir einleuchtet, schlage ich hingegen dies vor:
Lasst es uns über einen Parameter bei der parse-Methode konfigurierbar machen, Defaultwert aber entsprechend unserem aktuellem Verhalten. Aber beim Rendern der Markdown-Pages explizit das Verhalten umstellen.
Es kann dann also sein, dass manche Readmes im Backend erstmal falsch dargestellt werden, wenn Addons explizit auf das alte Verhalten gesetzt haben. Aber zumindest im Frontend, wenn da direkt mit rex_markdown gearbeitet wird, ändert sich dort das Rendering nicht.

@christophboecker
Copy link
Contributor Author

Bin mir nicht sicher, ob ich alles verstanden habe.

  1. Entwickler hat Kontrolle über den Core und ruft selbst parse() bzw. parse_with_toc() auf. Das kann er den zusätzlichen Parameter setzen. Saubere Lösung.

  2. Und bei der Anzeige der Readme.md schlägst Du vor, das Default-Verhalten auf "auf Markdown-Default zusetzen" zu setzen? Also bei

    [$toc, $content] = rex_markdown::factory()->parseWithToc(rex_file::get($path));
    den Zusatzparameter rein?

Was wäre auch was. Soll ich den PR anpassen?

@gharlan
Copy link
Member

gharlan commented Sep 25, 2020

Ja genau so meine ich es. Also für die Markdown-Pages im Backend wäre es also ein kleiner BC-Break. Allerdings geht es ja nur um die Darstellung, genauer um die Zeilenumbrüche. Das ist verkraftbar, denke ich. Vor allem da Readmes, die auf das alte Verhalten gesetzt haben, ja jetzt schon auf Github dann falsch dargestellt wurden. Es wäre also sowieso sinnvoll die betroffenen Readmes anzupassen.

Pr gerne so anpassen. Oder hat jemand anderes eine abweichende Meinung?

Zwei Konstanten SET_BREAKS_ENABLED und SET_BREAKS_DISABLED, die beim Aufruf des Parsers übergeben werdern können. Vorbelegung ist SET_BREAKS_ENABLED, so dass das gewohnte Verhalten weiter besteht, wenn nicht ausdrücklich beim Parser-Aufruf geändert.
@christophboecker
Copy link
Contributor Author

Hab es mal bei mir eingebaut und getestet. Sieht gut aus; hab es also in den PR übertragen. Du musst aber wohl noch mal nacharbeiten. Ich verstehe nicht was diese ganzen automatischen Tests anmeckern.

@staabm
Copy link
Member

staabm commented Oct 1, 2020

@christophboecker lass dich nicht irritieren von phpstan/psalm oder php-cs-fixer errors.. das bekommen wir einfach behoben, wenn das feature steht.

@staabm
Copy link
Member

staabm commented Oct 1, 2020

ausgenommen du willst es für dich lernen und machst es zur fortbildung :)

@gharlan gharlan added this to the REDAXO 5.12 milestone Oct 2, 2020
@gharlan
Copy link
Member

gharlan commented Oct 2, 2020

  • CS gefixt
  • Bei der Readme-Ansicht über Packages -> Hilfe wurde der Parameter noch nicht verwendet
  • editorconfig angepasst, sodass bei Markdown-Dateien nun Trailing Whitespace erhalten bleibt (braucht man bei dem neuen Modus für Line Breaks, durch zwei Leerzeichen am Zeilenende

@staabm
Copy link
Member

staabm commented Oct 2, 2020

* editorconfig angepasst, sodass bei Markdown-Dateien nun Trailing Whitespace erhalten bleibt (braucht man bei dem neuen Modus für Line Breaks, durch zwei Leerzeichen am Zeilenende

hmm ist das normal? klingt komisch.. 2 leerzeichen am ende?

@gharlan
Copy link
Member

gharlan commented Oct 2, 2020

Das ist Standard-Markdown:

When you do want to insert a
break tag using Markdown, you end a line with two or more spaces, then type return.

https://daringfireball.net/projects/markdown/syntax#p

Ist hier auf Github in den Markdown-Dateien wie gesagt auch so, nur hier in Markdown-Kommentaren ist der andere Modus aktiv, wo Zeilenumbrüche direkt zu <br> werden, ohne die zwei Leerzeichen.

Habe ich so auch schon öfters deswegen in der editorconfig gesehen, zum Beispiel bei Laravel:
https://github.com/laravel/laravel/blob/master/.editorconfig#L11-L12

@staabm
Copy link
Member

staabm commented Oct 2, 2020

Ok mal sehen wie es funktioniert

@tbaddade
Copy link
Member

tbaddade commented Oct 2, 2020

Ich hoffe das ein Umbruch in den Dateien als Umbruch ausgegeben wird.
Es ist furchtbar mit den 2 Leerzeichen am Ende. Ich merke das grad immer wieder in Gitlab. Zweimal Space angetippt, ergibt nicht 2 Leerzeichen sondern ein Punkt und ein Leerzeichen (sicherlich von Browser zu Browser oder OS zu OS unterschiedlich.)

@gharlan
Copy link
Member

gharlan commented Oct 2, 2020

Ich hoffe das ein Umbruch in den Dateien als Umbruch ausgegeben wird.

Findest du nicht, dass wir uns möglichst gleich verhalten sollten zur Darstellung der Markdown-Dateien auf Github?
Die Readmes in den Addons dienen doch nun mal sowohl dem Repo auf Github, als auch der Hilfe in REDAXO.

@staabm
Copy link
Member

staabm commented Oct 2, 2020

Findest du nicht, dass wir uns möglichst gleich verhalten sollten zur Darstellung der Markdown-Dateien auf Github?

aber auf github werden umbrüche doch auch als umbrüche dargestellt, oder?

@gharlan
Copy link
Member

gharlan commented Oct 2, 2020

aber auf github werden umbrüche doch auch als umbrüche dargestellt, oder?

Wie oben erläutert nur hier in den Kommentaren, nicht auf Dateiebene.
Einfach hier mal vergleichen mit der Preview: https://github.com/redaxo/redaxo/edit/master/SECURITY.md
Die Umbrüche werden so nicht übernommen. Nur mit 2 Leerzeichen am Ende der Zeile.

@staabm
Copy link
Member

staabm commented Oct 2, 2020

Ok, jetzt versteh ich es. Wir reden von <br> vs. <p>.

Dann hab ich bisher nicht umbrüche (also BRs) gemacht und habe sie nie vermisst. Daher kenn ich das mit den 2 spaces nicht.

Kurzum: kann wg. mir so gemacht werden

@gharlan gharlan added the automerge Automatisch PR rebasen und mergen label Oct 2, 2020
@kodiakhq kodiakhq bot merged commit 33e09bd into redaxo:master Oct 2, 2020
@christophboecker
Copy link
Contributor Author

Mercie.

@staabm
Copy link
Member

staabm commented Oct 3, 2020

Danke Dir!

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.

None yet

4 participants