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

Bugs with indented code #465

Closed
pozitron57 opened this issue Jul 28, 2018 · 8 comments · Fixed by #887
Closed

Bugs with indented code #465

pozitron57 opened this issue Jul 28, 2018 · 8 comments · Fixed by #887
Assignees
Labels
bug client (Javascript) client code and CSS
Milestone

Comments

@pozitron57
Copy link
Contributor

pozitron57 commented Jul 28, 2018

  1. Indented with 4 spaces code is displayed without an indent when seen in the Edit mode.
  2. In admin panel such a code is displayed in one line.
  3. If the comment has been edited from admin panel, it is no more possible for commenter to remove it from the page: Failed to load resource: the server responded with a status of 403 () embed.min.js:18 Not authorized to remove this comment!. Not sure what the default behavior should be in this case.

I made a gif to illustrate these bugs. I find the most important that in the admin panel there is no line breaks, any comment displayed in one line.

@pozitron57
Copy link
Contributor Author

It seems that it can be solved by applying a css property white-space: pre-wrap to div.textarea or to div.text div for the admin panel.

@Jenselme
Copy link
Contributor

Jenselme commented May 5, 2019

I encountered this today: one comment was not formatted properly on site and I wanted to edit it in the admin. But in the admin, no line breaks are respected in the display and worst they are lost on save.

I gave your solution a try and it fixes display, which is nice. But if I edit the comment, add line breaks and saves, the line breaks are lost. From what I understand, it comes from the fact that we rely on a div with editableContent set to true. So, if I originally have:

<div id="isso-text-202">
Test 1
</div>

I'll get after the edit:

<div id="isso-text-202" class="" contenteditable="false">
<div>Test 1</div>
<div>Test 2</div>
</div>

The display is correct (because by default div are displayed as block elements) but when we extract the text with textContent, we loose the line breaks that are visually conveyed by the div.

I worked on a fast workaround for the admin: use textarea instead of editable divs. By default it is disabled so the user can't edit it. If you click on Edit it becomes editable. It seems to fix both the display and edit issues. It's not very pretty but it works. You can see it here: https://github.com/Jenselme/isso/commit/e5a1b2ac3b6692e7f1aabb8a1a21b8f028acd28c

To fix this in the "standard edit" on the website, I think you could do something similar: when the user is editing, we rely on a textarea. When not (comment saved or in preview), we display the rendered HTML. We could also apply this method to the admin to view rendered comments by default and still be able to edit them correctly.

Let me know what you think. If it sounds like a good idea to you, I should be able to complete the fix for the admin and the standard edit page.

@Jenselme
Copy link
Contributor

Any thoughts on that?

@ix5 ix5 added bug needs-contributor Someone needs to implement this. Help wanted! labels Dec 27, 2021
@ix5 ix5 self-assigned this Dec 27, 2021
@BBaoVanC BBaoVanC moved this to Todo in Isso Todo May 14, 2022
@ix5 ix5 added this to the backburner milestone May 14, 2022
@BBaoVanC BBaoVanC moved this from Todo to Backlog in Isso Todo May 14, 2022
@ix5
Copy link
Member

ix5 commented May 14, 2022

I thoroughly agree with @Jenselme - the way the whole editing is implemented as contentEditable is bound to cause pain. I converted the admin to use textarea a while back, see #793

This is currently the "solution" for converting between HTML and plaintext:
https://github.com/posativ/isso/blob/668882f6c551c0cb979139ad62b2a97a0c968914/isso/js/app/utils.js#L29-L42

@Jenselme if you could re-upload your work again (repo seems deleted), I believe you might have some better approaches than my spaghetti code.

@BBaoVanC BBaoVanC moved this from Backlog to Todo in Isso Todo May 14, 2022
@BBaoVanC
Copy link
Contributor

I wonder if maybe it would be nicer to replace the admin page with instead being able to log in as an admin on the comment section. Should I make an issue to track that?

@ix5
Copy link
Member

ix5 commented May 16, 2022

I wonder if maybe it would be nicer to replace the admin page with instead being able to log in as an admin on the comment section.

That'd be nice as an addition, but keeping the "traditional" admin still makes sense to me.

Should I make an issue to track that?

Feel free.

@BBaoVanC
Copy link
Contributor

The whole part about the indentation not being preserved when editing seems to be fixed if you just add white-space: pre-wrap to .isso-textarea. However it still inserts and removes random newlines in weird places, I guess there must be some mistake in the ugly HTML <-> plaintext solution that you sent above.

Editing anything on the admin page seems to work fine currently..? However there are drawbacks to replacing the postbox with a <textarea>. Commenters don't want to manually resize the text box, and trying to use math and JS to auto resize the textarea seems to be a bit janky and unreliable.

However, I looked at a few other examples (Commento, Remark42, Talkyard, Flarum, Discourse, even the box I'm typing in right now on GitHub) and they all use a <textarea> for their post input text (note all of them except Commento and Remark have a fixed size). So I think using a textarea must clearly be the way to go for inputting plaintext.

Maybe if we can style the <textarea> to be large enough by default, and/or try and get some working JS code to auto-resize the box height, we can avoid having to do crazy workarounds just to get a contenteditable <div> working.

In summary, I think @Jenselme's solution is probably best, so I'll try and do this on the main Isso post box, based on what seems to already work on the admin dashboard.

@BBaoVanC BBaoVanC moved this from Todo to In progress in Isso Todo May 21, 2022
@BBaoVanC BBaoVanC removed this from Isso Todo May 26, 2022
@BBaoVanC BBaoVanC moved this to In progress in Isso Todo May 26, 2022
@ix5 ix5 closed this as completed in #887 May 30, 2022
Repository owner moved this from In progress to Done in Isso Todo May 30, 2022
@Jenselme
Copy link
Contributor

I wasn't able to look into this earlier. I looked at #887 and it seems like something good 👍

@ix5 ix5 added client (Javascript) client code and CSS bug and removed bug needs-contributor Someone needs to implement this. Help wanted! labels Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug client (Javascript) client code and CSS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants