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

WIP: EasyMDE #9973

Closed
wants to merge 3 commits into from
Closed

WIP: EasyMDE #9973

wants to merge 3 commits into from

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Jan 25, 2020

Convert SimpleMDE to EasyMDE

As far as I can tell this works (haven't done a full test yet), but perhaps it's not ideal.

Ideally this would be done with npm, however I don't understand nearly enough of that wizardry to get it working. We need not only easymde and codemirror, but all of codemirror's addons/modes.

Since SimpleMDE wasn't too pervasive yet, I decided to also rename variables...

@silverwind
Copy link
Member

Thanks for doing this.

Ideally this would be done with npm, however I don't understand nearly enough of that black wizardry to get it working

Vendoring is fine. We don't have a proper concept to lazy-load based on server's decision yet, but I think I will figure out something and then it can be easily un-vendored.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 25, 2020
<script src="{{StaticUrlPrefix}}/vendor/plugins/simplemde/simplemde.min.js"></script>
{{if .RequireEasyMDE}}
<script src="{{StaticUrlPrefix}}/vendor/plugins/easymde/easymde.min.js"></script>
<script src="{{StaticUrlPrefix}}/vendor/plugins/codemirror/codemirror.js"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Why did it work without CodeMirror being loaded before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be perfectly honest I have no clue, but there is an open PR (that looks unlikely to be merged) regarding it.
Ionaru/easy-markdown-editor#76

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think their method of loading CodeMirror would work in webpack but with this method, we need to manually load it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you should swap the order of these script tags thought.

@silverwind
Copy link
Member

Needs a rebase.

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@zeripath
Copy link
Contributor

@jolheiser I hope you don't mind I rebased for you.

@zeripath
Copy link
Contributor

I can't seem to be able to get a new line on the current head.

@codecov-io
Copy link

Codecov Report

Merging #9973 into master will decrease coverage by 0.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9973      +/-   ##
==========================================
- Coverage   42.26%   42.25%   -0.02%     
==========================================
  Files         610      610              
  Lines       80370    80370              
==========================================
- Hits        33971    33959      -12     
- Misses      42221    42232      +11     
- Partials     4178     4179       +1
Impacted Files Coverage Δ
routers/repo/setting.go 14.99% <0%> (ø) ⬆️
routers/repo/pull.go 29.06% <0%> (ø) ⬆️
routers/repo/issue.go 38% <100%> (ø) ⬆️
routers/repo/compare.go 40.44% <100%> (ø) ⬆️
routers/repo/wiki.go 38.88% <100%> (ø) ⬆️
routers/repo/editor.go 26.75% <66.66%> (ø) ⬆️
services/pull/update.go 51.16% <0%> (-3.49%) ⬇️
models/unit.go 37.03% <0%> (-2.47%) ⬇️
services/pull/check.go 54.54% <0%> (-2.1%) ⬇️
services/pull/patch.go 66.03% <0%> (-1.89%) ⬇️
... and 2 more

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 a83c373...382fc2f. Read the comment docs.

@silverwind
Copy link
Member

silverwind commented Mar 7, 2020

FYI regarding webpack: I attempted to webpack-build simplemde, but I think at least the codemirror addons/modes are not bundleable because loadmode does require(file) in the middle of it's code which webpack can not process.

What may work is to just copy the CM files and set modeURL and write a custom version of loadmode in our code that downloads and executes the scripts.

@silverwind
Copy link
Member

Related: #10729

@stale
Copy link

stale bot commented May 14, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label May 14, 2020
@silverwind
Copy link
Member

SimpleMDE will be ripped out eventually, so I guess we can close this.

@stale stale bot removed the issue/stale label May 14, 2020
@jolheiser
Copy link
Member Author

Agreed, closing this.

@jolheiser jolheiser closed this May 14, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants