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

Use <pre> for comments in admin area #604

Merged
merged 1 commit into from Dec 24, 2021
Merged

Conversation

ix5
Copy link
Collaborator

@ix5 ix5 commented Dec 28, 2019

Also update admin.css so that lines get wrapped

Note that document.getElementById().textContent() already overlooks the <pre> tags, so there is no need to strip them when using send_edit().

This is a dirty fix, but adding the client-side contenteditable mechanism here is something I'm not prepared to do. If anyone wants to pick this up, feel free to do so!

Also update admin.css so that lines get wrapped
@ix5 ix5 marked this pull request as ready for review Jun 3, 2020
@ix5 ix5 changed the title [WIP] Use <pre> for comments in admin area Use <pre> for comments in admin area Jun 3, 2020
@ix5
Copy link
Collaborator Author

ix5 commented Jun 3, 2020

Hmm, since it seems no one has stepped up so far and provided a proper solution, I guess this is a good-enough stopgap one.

See #570

@hamoid
Copy link

hamoid commented Jun 3, 2020

Hi! Does this save the <pre> with the comment? And if you edit it multiple times, adds it each time?

Wouldn't it be better to do <div id="isso-text-{{comment.id}}" class="pre"> and create a .pre css style? (I may be totally wrong)

@ix5
Copy link
Collaborator Author

ix5 commented Jun 3, 2020

Hi! Does this save the <pre> with the comment? And if you edit it multiple times, adds it each time?

We have a saying in German: "He who is able to read has the clear advantage".
See in OP:

Note that document.getElementById().textContent() already overlooks the <pre> tags, so there is no need to strip them when using send_edit().

So rest assured, I already tested that "case" and am running it locally - it's working just fine.

Wouldn't it be better to do <div id="isso-text-{{comment.id}}" class="pre"> and create a .pre css style?

Well you want the browser to explicitly render and treat internally the text field as a <pre> element so that with the contenteditable property, you can edit the contents directly while preserving the line breaks etc.
If you only style it with pre styles, your nice manualy formatting, including the line breaks, will get stripped out, because in HTML, any amount of whitespace is collapsed into one.

@hamoid
Copy link

hamoid commented Jun 3, 2020

And he who has time to read has even more advantage :-P

@ix5
Copy link
Collaborator Author

ix5 commented Oct 10, 2020

@jelmer thoughts?

@jelmer
Copy link
Collaborator

jelmer commented Oct 10, 2020

Why <pre> rather than proper rendering?

@ix5
Copy link
Collaborator Author

ix5 commented Oct 11, 2020

Why <pre> rather than proper rendering?

Because then you'd have to deal with switching back and forth between editing markdown source and rendered HTML inside the admin interface - or worse, implement some variation of contenteditable with something like CKEditor or QuillJS.

IMO the admin interface should be kept simple, and admins can be expected to edit markdown themselves.

@ix5
Copy link
Collaborator Author

ix5 commented Jan 15, 2021

@fluffy-critter since you're taking a look at issues anyway, what are your thoughts? This is way easier than implementing the render functionality of the client embed js in admin once again.

@fluffy-critter
Copy link
Contributor

fluffy-critter commented Jan 15, 2021

I don't have any particular thoughts about this aspect.

@jelmer
Copy link
Collaborator

jelmer commented Dec 24, 2021

Okay, let's land this for the moment in lieu of a proper fix

jelmer
jelmer approved these changes Dec 24, 2021
@jelmer jelmer merged commit 800601a into posativ:master Dec 24, 2021
1 check passed
@ix5 ix5 deleted the admin-plaintext branch Dec 24, 2021
@ix5 ix5 self-assigned this Feb 6, 2022
@ix5 ix5 added feature needs-contributor Someone needs to implement this. Help wanted! labels Feb 6, 2022
@ix5
Copy link
Collaborator Author

ix5 commented Feb 6, 2022

In conjuntion with 2391e8e, this PR has opened up another annoying bug when editing comments from inside the admin area, appending literal <pre> tags when editing.

The admin really shouldn't be using contenteditable but rather proper forms, else we're opening a whole can of worms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature needs-contributor Someone needs to implement this. Help wanted!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants