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

Switch from SimpleMDE to EasyMDE #13333

Merged
merged 9 commits into from
Nov 10, 2020
Merged

Conversation

zeripath
Copy link
Contributor

Signed-off-by: Andrew Thornton art27@cantab.net

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Oct 27, 2020
@zeripath zeripath added this to the 1.14.0 milestone Oct 27, 2020
@zeripath
Copy link
Contributor Author

This could do with a few people testing it to ensure that it actually is doing the right thing and has set up the codemirror properly

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 27, 2020
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@silverwind
Copy link
Member

Some warning about CodeMirror seen during make watch:

WARNING in ./node_modules/codemirror/addon/mode/loadmode.js 53:6-19
Critical dependency: the request of a dependency is an expression
 @ ./web_src/js/easymde.js
 @ multi ./web_src/js/easymde.js ./node_modules/easymde/dist/easymde.min.css

Not sure what's the issue but CM is generally rather webpack-unfriendly unfortunately.

@zeripath
Copy link
Contributor Author

zeripath commented Oct 27, 2020

Some warning about CodeMirror seen during make watch:

WARNING in ./node_modules/codemirror/addon/mode/loadmode.js 53:6-19
Critical dependency: the request of a dependency is an expression
 @ ./web_src/js/easymde.js
 @ multi ./web_src/js/easymde.js ./node_modules/easymde/dist/easymde.min.css

Not sure what's the issue but CM is generally rather webpack-unfriendly unfortunately.

I think it's to do with the load-addon plugin. I'll see if we can move loading that out to the script section.

(When I googled the error people said to just ignore it...)

@silverwind
Copy link
Member

arc-green buttons need some fixes (class rename?):

image

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

That seems to do it - but I don't know how to test the mode loading stuff.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@silverwind
Copy link
Member

how to test the mode loading stuff.

Not sure how the initialization goes but I think easymde should trigger the markdown mode load itself and you should see some syntax highlighting when it does. Probably need to vendor codemirror for it to work because codemirrors hates webpack.

@silverwind
Copy link
Member

Thought I would not mind if we do away with the syntax highlight (and CM modes) in comment boxes, it's not really something we'll have in the long run anyways.

@zeripath
Copy link
Contributor Author

So one of the cool things about easymde is that it appears to work on android...

It might a be reason to allow users to choose between codemirror and monaco editors. (monaco fails miserably on android.)

@a1012112796

This comment has been minimized.

@silverwind

This comment has been minimized.

@a1012112796

This comment has been minimized.

@silverwind

This comment has been minimized.

@a1012112796
Copy link
Member

suggest use it in release content editor also.

@zeripath
Copy link
Contributor Author

The way this PR is written it simple replaces any call to SimpleMDE as EasyMDE

@silverwind
Copy link
Member

suggest use it in release content editor also.

In another PR please. This is only a replacement.

@silverwind
Copy link
Member

silverwind commented Oct 29, 2020

Gave it a quick test, seems to work as before.

I think we can actually drop the loadmode.js, meta.js and modeURL handling in footer.tmpl because I think those are relics from the past when codemirror was used as code editor. Right now it only does markdown mode so it should never need to load anything else.

Also, with this I think we can nuke the codemirror vendored files in public.

@silverwind
Copy link
Member

You can pull in silverwind@fa97212 which nukes the codemirror vendoring, I did not notice any difference in the comment editor without it.

@zeripath
Copy link
Contributor Author

Let's not do that in this PR - I think we could do that in a week if we don't notice any problems with the EasyMDE replacement - it would be useful to know/ensure that easymde is doing everything it should do first.

@silverwind
Copy link
Member

Ok we can clean that up later too.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 30, 2020
@silverwind
Copy link
Member

silverwind commented Oct 30, 2020

Think we need to disable import/no-extraneous-dependencies as that rule does not seem to have an option to allow resolvable dependencies (import-js/eslint-plugin-import#1934).

@silverwind
Copy link
Member

Or we just nuke the mode load stuff so we don't even need to do such workarounds. As I understand it, codemirror inside easymde is pretty-much zero-conf and does not need to load modes.

@silverwind
Copy link
Member

I'd suggest you apply below and git rm -rf public/vendor/plugins/codemirror.

diff --git a/templates/base/footer.tmpl b/templates/base/footer.tmpl
index c8e9674fb..00a643f66 100644
--- a/templates/base/footer.tmpl
+++ b/templates/base/footer.tmpl
@@ -12,13 +12,8 @@

 	{{template "base/footer_content" .}}
 {{if .RequireSimpleMDE}}
 	<script src="{{StaticUrlPrefix}}/js/easymde.js?v={{MD5 AppVer}}"></script>
-	<script src="{{StaticUrlPrefix}}/vendor/plugins/codemirror/addon/mode/loadmode.js"></script>
-	<script src="{{StaticUrlPrefix}}/vendor/plugins/codemirror/mode/meta.js"></script>
-	<script>
-		CodeMirror.modeURL =  "{{StaticUrlPrefix}}/vendor/plugins/codemirror/mode/%N/%N.js";
-	</script>
 {{end}}

 <!-- Third-party libraries -->
 {{if .RequireU2F}}
diff --git a/web_src/js/easymde.js b/web_src/js/easymde.js
index 39ead02c1..c2f608b55 100644
--- a/web_src/js/easymde.js
+++ b/web_src/js/easymde.js
@@ -1,8 +1,4 @@
 import EasyMDE from 'easymde';

-import CodeMirror from 'codemirror/lib/codemirror.js';
-
 window.EasyMDE = EasyMDE;
 window.SimpleMDE = EasyMDE;
-window.CodeMirror = CodeMirror;
-

@zeripath
Copy link
Contributor Author

AFAIU we still need the codemirror to be exposed.

@silverwind
Copy link
Member

silverwind commented Oct 30, 2020

I don't see a purpose in doing that, that exposure was for the code editor to load modes which we've since switched to monaco. Again, I don't think easymde's codemirror needs the ability to load modes.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

the githook editor is still codemirror I think.

@zeripath
Copy link
Contributor Author

Let's get the easymde in first then sort out the rest of these issues in another PR

@silverwind
Copy link
Member

silverwind commented Oct 30, 2020

Right, git hook needs to migrate to Monaco, it's interfacing with window.CodeMirror and loading the shell mode, good find.

@codecov-io
Copy link

Codecov Report

Merging #13333 (ce38d9e) into master (542edc2) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13333   +/-   ##
=======================================
  Coverage   42.18%   42.19%           
=======================================
  Files         694      694           
  Lines       76351    76351           
=======================================
+ Hits        32210    32215    +5     
+ Misses      38861    38859    -2     
+ Partials     5280     5277    -3     
Impacted Files Coverage Δ
models/error.go 35.22% <0.00%> (-0.51%) ⬇️
modules/indexer/stats/db.go 52.17% <0.00%> (ø)
modules/git/repo.go 45.68% <0.00%> (+0.50%) ⬆️
models/gpg_key.go 53.90% <0.00%> (+0.57%) ⬆️
modules/process/manager.go 75.00% <0.00%> (+2.50%) ⬆️
modules/git/utils.go 77.04% <0.00%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9155f13...ce38d9e. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 10, 2020
@techknowlogick techknowlogick merged commit 13b8c0b into go-gitea:master Nov 10, 2020
@zeripath zeripath deleted the easymde branch November 10, 2020 21:17
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants