js, templates: Replace contenteditable div with textarea

[BBaoVanC]

Checklist

• All new and existing tests are passing
• (If adding features:) I have added tests to cover my changes
• (If docs changes needed:) I have updated the documentation accordingly. N/A?
• I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
• I have written proper commit message(s)
  

What changes does this Pull Request introduce?

• Replace the contenteditable div with a proper textarea - fix indentation bugs

Why is this necessary?

Fixes #465

[ix5]

Hi @BBaoVanC, very nice effort, thank you! This is sorely needed.

A couple thoughts of mine:

We shouldn't add any external dependencies, instead vendor them in (the ES5-compatible version). We don't need e.g. (afaics!)

• amd/module sniffing
• the lifecycle events like autosize:resized, autosize:update etc.
• things like the destroy function
• the environment sniffing (nodejs/IE8, we don't support running in either, and use JSDOM for running DOM-based tests anyway)

We should set the max length HTML5 attribute for textarea (and also email and website)
https://github.com/posativ/isso/blob/46c141c9517665620929192b05d2f41275bbaefd/isso/views/comments.py#L166-L179
(And those error message responses are just plain bad - improve them and their handling on the client side as well, in another PR)

The autosize FAQ suggests disabling blurs on Safari for textarea - good idea to add to the CSS.

---

For yourself: Best to push proper commits already, that way you don't have so much explaining to do in the PR, you can just copy-paste from your commit messages.

[BBaoVanC]

Thanks for the response @ix5

How would I vendor the dependencies like you said? I tried to search the repository for the word vendor but I don't see anything.

Also about the commit messages, I'll rebase it later once it's in less of a draft stage. And in this stage with all the changes I'm trying to do it's easier to just write down the changes I make in the PR description as I go instead of writing it on every commit.

[ix5]

How would I vendor the dependencies like you said? I tried to search the repository for the word vendor but I don't see anything.

Just put the file into app/lib/autosize.js (with proper license information, ofc).

Also about the commit messages, I'll rebase it later once it's in less of a draft stage. And in this stage with all the changes I'm trying to do it's easier to just write down the changes I make in the PR description as I go instead of writing it on every commit.

Dw, just something I found useful for myself.

[BBaoVanC]

I decided that it's best to try and replace the "Edit" box with a postbox in a different PR. It would require a large refactor and I don't want to block this change for that long. As far as I can tell, editing indentation works fine in its current state.

I removed the editorify() test since that function isn't needed anymore, but for some reason the screenshots don't match the CI. Could also benefit from a couple tests, which I'll look into tomorrow, in addition to rebasing and adding to CHANGES.rst.

Also, I feel like this project could benefit from a lot of the client code being refactored and/or rewritten. I wonder if it would be worth using TypeScript? I'll play around with this a bit in a different branch on my fork.

[ix5]

I decided that it's best to try and replace the "Edit" box with a postbox in a different PR. It would require a large refactor and I don't want to block this change for that long. As far as I can tell, editing indentation works fine in its current state.

That's sensible, I agree.

I removed the editorify() test since that function isn't needed anymore, but for some reason the screenshots don't match the CI. Could also benefit from a couple tests, which I'll look into tomorrow, in addition to rebasing and adding to CHANGES.rst.

Yeah, especially the comment.png screenshot shouldn't change, it appears it changes height by 1px. Weird. Maybe some outline sizing gets transferred.

Also, I feel like this project could benefit from a lot of the client code being refactored and/or rewritten. I wonder if it would be worth using TypeScript? I'll play around with this a bit in a different branch on my fork.

If we decide to move to Typescript, we'd have to do a whole lot of other refactoring as well. I feel like the current state doesn't merit introducing types - it'd be like smearing lipstick on a pig. We'd also have a whole rat's tail of development dependencies to contend with. See #834 for some more of my thoughts on the topic. I'm not opposed, I just feel like it should be worth it. E.g. remark42's client code is extremely complicated and inaccessible (to me).

For refactoring the code, have a look at https://github.com/isso-comments/client-experimental - maybe we can collaborate a bit there, while keeping ES5 compatibility.

Edit: Please also see #890, I hope this makes the whole screenshot testing handling a bit easier to understand. Should I change anything to make it clearer/friendlier? Still figuring CI stuff out.

[BBaoVanC]

If we decide to move to Typescript, we'd have to do a whole lot of other refactoring as well.

I feel like this could be beneficial overall, since I'm a big fan of statically typed languages (it feels less awkward and overall more stable/definite to work in). Code is easier to follow with defined types.

It looks like with TypeScript + Webpack, we would be able to write with modern syntax, but have it transpile to ES5 compatible output. We also don't have to completely refactor everything at once since, as far as I can tell regular JS code is completely valid in TS (just you don't get the compile-time error-checking that you do with TS).

I'll look into your experimental client in a bit, but first: do we have some way to run the tests on the main Isso repo over here on the experimental client? I feel like that would be a nice way to make sure during development that the experimental client functions the same. Hopefully we could gradually add more tests, and if there's any unintended functionality on the main client which is discovered, it can be fixed.

We'd also have a whole rat's tail of development dependencies to contend with.

I'm not too concerned about having dependencies, I feel that NPM is just sort of designed in a way that makes it hard to avoid having tons of packages/dependencies. As long as the ones we explicitly list in package.json are few, I think it should be ok. But I'll have to see.

---

On this PR: tests seem to work except the integration one failed with a weird error:

docker: Error response from daemon: cannot join network of a non running container: 04b9d43d9b947d15a78937f235c5307d37b8d1f02a08225259d083b0cbcdd0f0.

But it worked fine on my own machine (I used similar steps to the E2E workflow on my system to update the screenshots). I did rebase this PR onto the latest master before updating screenshots again, but also I don't really see any changes in this PR that would break that.

I'm about to rebase it again to make the commit messages better, which will trigger the CI again, so we can see if it's a random one-off error.

Looks like the same error happened on the latest commit to master, so I guess it's not specific to this PR.

[ix5]

The PR as it currently stands is done, as far as I can see. I know you'll fix the other issues we've talked about in a later PR.
In the interest of this not growing into a never-ending back-and-forth, I'll merge as-is (unless you have objections). I (force-)pushed a rebase and a small nit (upon leaving preview, focus textarea).

Re: Typescript etc: I'll open another issue for that so we can continue discussion there.

[BBaoVanC]

The PR as it currently stands is done, as far as I can see. I know you'll fix the other issues we've talked about in a later PR. In the interest of this not growing into a never-ending back-and-forth, I'll merge as-is (unless you have objections). I (force-)pushed a rebase and a small nit (upon leaving preview, focus textarea).

Re: Typescript etc: I'll open another issue for that so we can continue discussion there.

Sounds good to me, I don't see anything else that needs to be changed either.

[ix5]

New issue opened #894

I feel like this could be beneficial overall, since I'm a big fan of statically typed languages (it feels less awkward and overall more stable/definite to work in). Code is easier to follow with defined types.

It looks like with TypeScript + Webpack, we would be able to write with modern syntax, but have it transpile to ES5 compatible output. We also don't have to completely refactor everything at once since, as far as I can tell regular JS code is completely valid in TS (just you don't get the compile-time error-checking that you do with TS).

TS sure looks like a nice addition, but we might also be limiting ourselves in the contributors we get - they'd have to know TS as well. This kind of feels a bit like the "sprinkle some async" fad in python to me. Maybe start with better test coverage and work from there?

I'll look into your experimental client in a bit, but first: do we have some way to run the tests on the main Isso repo over here on the experimental client? I feel like that would be a nice way to make sure during development that the experimental client functions the same. Hopefully we could gradually add more tests, and if there's any unintended functionality on the main client which is discovered, it can be fixed.

You can simply swap out the JS part for the integration tests (copy the generated embed.dev.js from the experimental client). Or do as suggested in the README and cd dist && python -m http.server 8000 and change the embed URL to localhost:8000/embed.dev.js:

--- a/isso/demo/index.html
+++ b/isso/demo/index.html
@@ -9,8 +9,8 @@
   <div id="wrapper" style="max-width: 900px; margin-left: auto; margin-right: auto;">
    <h2><a href="index.html">Isso Demo</a></h2>
 
-   <script src="../js/embed.dev.js"
-           data-isso="../"
+   <script src="http://localhost:8000/embed.dev.js"
+           data-isso="http://localhost:8080/"
            ></script>
    <!-- Uncomment to only test count functionality: -->
    <!-- <script src="../js/count.dev.js"></script> -->

Then run npm run test-integration from the "old" isso repo.

The repo also has its own unit tests (since too much has changed). npm run test-unit should work from the new repo.

I'm not too concerned about having dependencies, I feel that NPM is just sort of designed in a way that makes it hard to avoid having tons of packages/dependencies. As long as the ones we explicitly list in package.json are few, I think it should be ok. But I'll have to see.

I'm already kind of annoyed at all the Javascript packages constantly jumping sub-revisions (e.g. Jest and puppeteer all jumped several since I added them, and they need to be kept in sync with system chromium version if you don't use the .local-chromium version). You're never quite sure if something just broke because of a version change of a dep when re-installing or you broke something yourself. webpack is already quite a moving target and has not been pleasant to set up. I'm not really looking forward to spending maintainer time accepting PRs bumping revisions from bots every few days (new tsc release here, new babel release there, ...). It's not too much to do, but death by a thousand cuts.