New editor #458

trinity-1686a · 2019-03-05T15:19:10Z

elegaanz 评论于

2019-03-05 15:19:10 +00:00

(从 github.com 迁移)

With this PR, when JS is activated and WASM supported, the article editor will be dynamically replaced with contenteditables elements. This makes the editing interface simpler and less like a regular form. It will also allow us to easily add visual formatting with native browser APIs (and to insert images or videos directly). Here is a little demo:

There is still a lot to do, but it is a good first step.

Fixes #255

With this PR, when JS is activated and WASM supported, the article editor will be dynamically replaced with `contenteditable`s elements. This makes the editing interface simpler and less like a regular form. It will also allow us to easily add visual formatting with native browser APIs (and to insert images or videos directly). Here is a little demo: ![peek 05-03-2019 16-12](https://user-images.githubusercontent.com/16254623/53815536-1dc05680-3f62-11e9-94d3-b363ed84eb97.gif) There is still a lot to do, but it is a good first step. Fixes #255

codecov[bot] 评论于

2019-03-05 18:54:29 +00:00

(从 github.com 迁移)

Codecov Report

Merging #458 into master will increase coverage by 1.64%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   26.49%   28.13%   +1.64%     
==========================================
  Files          64       64              
  Lines        7111     7496     +385     
==========================================
+ Hits         1884     2109     +225     
- Misses       5227     5387     +160

# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/458?src=pr&el=h1) Report > Merging [#458](https://codecov.io/gh/Plume-org/Plume/pull/458?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/2a188abfa152efe7a2794174279e3b9da4a809d1?src=pr&el=desc) will **increase** coverage by `1.64%`. > The diff coverage is `n/a`. ```diff @@ Coverage Diff @@ ## master #458 +/- ## ========================================== + Coverage 26.49% 28.13% +1.64% ========================================== Files 64 64 Lines 7111 7496 +385 ========================================== + Hits 1884 2109 +225 - Misses 5227 5387 +160 ```

elegaanz 评论于

2019-03-05 19:46:53 +00:00

(从 github.com 迁移)

Something I just noticed: published articles are always drafts (unless you are editing an already published article)

trinity-1686a 评论于

2019-03-06 17:09:24 +00:00

管理员

I'm having trouble making it work. I had to tinker plume-front.js. Any idea why, or how to prevent that?

elegaanz 评论于

2019-03-06 17:25:23 +00:00

(从 github.com 迁移)

Was the build script correctly run by cargo? Like, did you saw plume(build) before plume(bin) next to the compilation progress bar? Otherwise, you can try to add a println!("cargo:rerun-if-changed=target/deploy/plume-front.js") (IIRC) to it, so that we are sure it is re-run when the JS changes (it does when the WASM changes normally, but maybe that's not enough).

Was the build script correctly run by cargo? Like, did you saw `plume(build)` before `plume(bin)` next to the compilation progress bar? Otherwise, you can try to add a `println!("cargo:rerun-if-changed=target/deploy/plume-front.js")` (IIRC) to it, so that we are sure it is re-run when the JS changes (it does when the WASM changes normally, but maybe that's not enough).

trinity-1686a 评论于

2019-03-06 18:09:49 +00:00

管理员

ok my bad, I ran cargo web deploy -o ../static so the buildscript did not update the js file as it should have

ok my bad, I ran `cargo web deploy -o ../static` so the buildscript did not update the js file as it should have

👍 1

trinity-1686a 评审于 2019-03-06 22:57:35 +00:00

plume-front/src/editor.rs

trinity-1686a 评论于

2019-03-06 22:57:34 +00:00

管理员

By doing the following change, I was able to copy past a picture in a post and have it rendered as expected. However when re-editing the article, < and > where escaped, leading to a messy html tag in the middle of the content. I don't know if this is considered in scope for this pr, but this might be something to look at, as it will get important once the editor get richer

                            set_value("editor-content", js!{return @{&widgets.2}.innerHTML}.as_str().unwrap_or_default());

By doing the following change, I was able to copy past a picture in a post and have it rendered as expected. However when re-editing the article, `<` and `>` where escaped, leading to a messy html tag in the middle of the content. I don't know if this is considered in scope for this pr, but this might be something to look at, as it will get important once the editor get richer ```suggestion set_value("editor-content", js!{return @{&widgets.2}.innerHTML}.as_str().unwrap_or_default()); ```

trinity-1686a 评审于 2019-03-06 23:00:27 +00:00

po/plume-front/en.po~

trinity-1686a 评论于

2019-03-06 22:58:46 +00:00

管理员

should this file (and fr.po~) exists?

igalic (从 github.com 迁移) 评审于 2019-03-07 07:25:45 +00:00

po/plume-front/en.po~

igalic (从 github.com 迁移) 评论于

2019-03-07 07:25:44 +00:00

my suggestion to @BaptisteGelez is to either add this our .gitignore or to her global .git/ignore

my suggestion to @BaptisteGelez is to either add this our `.gitignore` or to her global `.git/ignore`

elegaanz (从 github.com 迁移) 评审于 2019-03-07 08:52:24 +00:00

po/plume-front/en.po~

elegaanz (从 github.com 迁移) 评论于

2019-03-07 08:52:24 +00:00

No, I though *.po~ was in the gitignore, but it looks like it is not. 🤷‍♀️ I will add it.

No, I though `*.po~` was in the gitignore, but it looks like it is not. :woman_shrugging: I will add it.

elegaanz (从 github.com 迁移) 评审于 2019-03-07 08:52:58 +00:00

plume-front/src/editor.rs

elegaanz (从 github.com 迁移) 评论于

2019-03-07 08:52:58 +00:00

I think it would be easy to avoid escaping here, I will try.

trinity-1686a 评审于 2019-03-07 08:55:09 +00:00

po/plume-front/en.po~

trinity-1686a 评论于

2019-03-07 08:55:09 +00:00

管理员

They are only ignored in some directories, not everywhere

elegaanz 评论于

2019-03-07 10:43:21 +00:00

(从 github.com 迁移)

I added back the character counter, but it is not as precise as the previous one, since I only took the article content in account (otherwise I would have had to rebuild all the original form every time, which would be quite long both in code and when running). I don't think that's a big issue, since the previous one was not perfectly precise neither, and that people will rarely reach this limit anyway.

trinity-1686a 评审于 2019-03-13 09:35:37 +00:00

trinity-1686a 留下了一条评论

管理员

mostly code-style comment again

.gitignore

trinity-1686a 评论于

2019-03-13 09:21:59 +00:00

管理员

this line is duplicated

plume-front/src/editor.rs

trinity-1686a 评论于

2019-03-13 09:30:05 +00:00

管理员

this closure is really long. It would gain in readability to be divided in multiple smaller functions called one after an other (like "hide non-js form", "add js editor", "show popup", "copy editor to form"...). I understand what it's doing, but if there is something wrong, it's too complex to see it right away

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

trinity-1686a 评论于

2019-03-13 09:32:27 +00:00

管理员

I love that it just works like in the back-end, with nothing more to

elegaanz (从 github.com 迁移) 评审于 2019-03-13 14:38:29 +00:00

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

elegaanz (从 github.com 迁移) 评论于

2019-03-13 14:38:28 +00:00

Yeah, that's really great! The only downside is that we include all the translations in the .wasm so it is a bit bigger, but there is like 3 or 4 messages for the moment so I guess it's fine (and I don't expect it to become much bigger, I think we will never have more than 30 messages for the front end).

trinity-1686a 评审于 2019-03-13 15:37:00 +00:00

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

trinity-1686a 评论于

2019-03-13 15:36:59 +00:00

管理员

considering all Plume translations (not plume-front) are 208k on my computer, any picture loaded on the front page must cost much more than having all translations available

elegaanz 评论于

2019-03-13 18:09:38 +00:00

(从 github.com 迁移)

I tried to make the code a bit more readable, but I'm not sure it is enough…

igalic (从 github.com 迁移) 评审于 2019-03-13 18:23:13 +00:00

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

igalic (从 github.com 迁移) 评论于

2019-03-13 18:23:13 +00:00

so how big is the dynamic front-end right now? and how much of that is cachable?

elegaanz (从 github.com 迁移) 评审于 2019-03-13 18:31:13 +00:00

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

elegaanz (从 github.com 迁移) 评论于

2019-03-13 18:31:13 +00:00

It is 2.2M on my computer, but there are various ways to optimize WASM size that we didn't explored yet. And I don't know how much of it is cachable? Nothing?

trinity-1686a 评审于 2019-03-13 19:21:42 +00:00

plume-front/src/main.rs

					
				@ -6,0 +20,4 @@

				lazy_static! {

				    static ref CATALOG: gettext::Catalog = {

				        let catalogs = include_i18n!();

				        let lang = js!{ return navigator.language }.into_string().unwrap();

trinity-1686a 评论于

2019-03-13 19:21:42 +00:00

管理员

I'd say everything, it's a static file after all. I tried compiling with cargo web deploy --release, it seems to work, and is "only" 832k. We could also try to compact plume-front.js as it's 35k with indentation and things, however I'm not sure it's that important

I'd say everything, it's a static file after all. I tried compiling with `cargo web deploy --release`, it seems to work, and is "only" 832k. We could also try to compact plume-front.js as it's 35k with indentation and things, however I'm not sure it's that important

trinity-1686a 于 2019-03-15 15:01:55 +00:00 批准此合并请求

trinity-1686a 留下了一条评论

管理员

👍

:+1:

trinity-1686a 评论于

2019-03-15 15:08:39 +00:00

管理员

Oups, 2 little things : It doesn't seems to be possible to add carriage return in the content (prevent default I guess), and it may be nice to have a button to switch back to the plain markdown editor

登录并参与到对话中。

无评审员

未选择里程碑

暂无项目

未指派成员

2 位参与者

通知

到期时间

到期日期无效或超出范围。请使用“yyyy-mm-dd”格式。

未设置到期时间。

依赖工单

没有设置依赖项。

引用：Plume/Plume#458

行数
列数