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 code editor to Monaco #11366

Merged
merged 25 commits into from
May 14, 2020
Merged

Switch code editor to Monaco #11366

merged 25 commits into from
May 14, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented May 10, 2020

This switches out CodeMirror for Monaco which is based on the same code base as VS Code and should work pretty similar to it.

It does add a few async chunks, totalling around 10MB to our build. It currently supports around 65 languages and in the default configuration, each language would emit one ugly [number].js chunk, so I opted to combine them all into a single 4MB file for now.

CodeMirror is still being used under the hood by SimpleMDE so it can not be removed yet.

Screen Shot 2020-05-10 at 22 28 49

Screen Shot 2020-05-10 at 22 27 26

@silverwind
Copy link
Member Author

silverwind commented May 10, 2020

TODO:

  • Refactors
  • Maybe use it for Markdown files too
  • Maybe disable the minimap and other undesirable features to reduce bundle size
  • Figure out why IntelliSense is not working
  • Remove extra fetch for editorconfig, render it in the template

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 10, 2020
@lafriks lafriks added the type/enhancement An improvement of existing functionality label May 10, 2020
@silverwind silverwind marked this pull request as draft May 11, 2020 18:40
@silverwind silverwind marked this pull request as ready for review May 11, 2020 19:18
@silverwind
Copy link
Member Author

Should be good for review now. I decided to not cut features. The bundles are big without a doubt, but it still loads within around 2 seconds on my machine.

@codecov-io
Copy link

codecov-io commented May 11, 2020

Codecov Report

Merging #11366 into master will decrease coverage by 0.00%.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11366      +/-   ##
==========================================
- Coverage   43.84%   43.83%   -0.01%     
==========================================
  Files         613      613              
  Lines       87229    87213      -16     
==========================================
- Hits        38242    38233       -9     
+ Misses      44266    44258       -8     
- Partials     4721     4722       +1     
Impacted Files Coverage Δ
routers/repo/editor.go 24.39% <38.46%> (+0.05%) ⬆️
modules/indexer/stats/queue.go 62.50% <0.00%> (-18.75%) ⬇️
modules/indexer/stats/db.go 40.62% <0.00%> (-9.38%) ⬇️
modules/git/utils.go 65.67% <0.00%> (-4.48%) ⬇️
routers/api/v1/repo/commits.go 77.95% <0.00%> (-2.81%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.93%) ⬇️
modules/git/repo.go 49.79% <0.00%> (-0.84%) ⬇️
routers/api/v1/api.go 76.85% <0.00%> (-0.04%) ⬇️
modules/markup/markdown/goldmark.go 51.92% <0.00%> (+0.36%) ⬆️
services/pull/pull.go 33.96% <0.00%> (+0.76%) ⬆️
... and 8 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 f8e015e...5cc2f5e. Read the comment docs.

@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 May 11, 2020
@lafriks lafriks added this to the 1.12.0 milestone May 11, 2020
@lafriks
Copy link
Member

lafriks commented May 11, 2020

This is soooo nice :)

@silverwind
Copy link
Member Author

There is a issue with the highlight.worker.js not being generated, don't land yet.

@silverwind
Copy link
Member Author

Actually it's fine, I was just misinterpreting webpack output. js/highlight.js is the worker generated from the chunk in web_src/js/features/highlight.js which includes the worker inline. This mechanism did not actually change with the PR.

@jolheiser
Copy link
Member

When creating a new file from the UI, it seemed to be perpetually "loading..."

@silverwind
Copy link
Member Author

Will fix that, did not try creating new files yet.

@silverwind
Copy link
Member Author

File creation fixed, it was related to bad error handling which I clean up. Also fixed editorconfig being missing in the template code after POST (for example, when filename already exists).

This switches out CodeMirror for Monaco which is based on the same code
base as VS code and should work pretty similar to it.

It does add a few async chunks, totalling around 10MB to our build. It
currently supports around 65 languages and in the default configuration,
each language would emit one ugly [number].js chunk, so I opted to
combine them all into a single file for now.

CodeMirror is still being used under the hood by SimpleMDE so it can not
be removed yet.
@silverwind
Copy link
Member Author

@jolheiser care to have another look?

@lunny
Copy link
Member

lunny commented May 14, 2020

minimap could be an option. For a large file, it could be disabled. For smaller files, it's beatiful!!!

@silverwind
Copy link
Member Author

Yes, I suppose we can create a small options UI later, especially for things that can not be controlled via .editorconfig. There are many such options.

But first, I think we'd need some kind of mechanism to store user UI settings in the database (maybe just a JSON object stored as string).

Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

LGTM

@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 May 14, 2020
@lafriks
Copy link
Member

lafriks commented May 14, 2020

make lg-tm work

@lafriks lafriks merged commit 9249c81 into go-gitea:master May 14, 2020
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label May 14, 2020
@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html lang="{{.Language}}">
<html lang="{{.Language}}" class="theme-{{.SignedUser.Theme}}">
Copy link
Member

Choose a reason for hiding this comment

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

How to handle non-login user?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think anonymous users have access to code edditor

Copy link
Member Author

@silverwind silverwind May 15, 2020

Choose a reason for hiding this comment

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

he's right, condition should be improved as even unsigned users should have a theme, just not in this variable. Right now it does not matter but I want to move the theme to CSS variables using this class so it has to work then.

ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Switch code editor to Monaco

This switches out CodeMirror for Monaco which is based on the same code
base as VS code and should work pretty similar to it.

It does add a few async chunks, totalling around 10MB to our build. It
currently supports around 65 languages and in the default configuration,
each language would emit one ugly [number].js chunk, so I opted to
combine them all into a single file for now.

CodeMirror is still being used under the hood by SimpleMDE so it can not
be removed yet.

* inline editorconfig, fix diff, use for markdown, remove more dead code

* refactors, remove jquery usage

* use tab_width

* fix intellisense

* rename function for clarity

* misc tweaks, enable webpack progress display

* only use --progress on dev build

* remove useless borders in arc-green

* fix typo

* remove obsolete comment

* small refactor

* fix file creation and various refactors

* unset useTabStops too when no editorconfig

* small refactor

* disable webpack's [big] warnings

* remove useless await

* fix dark theme check

* rename chunk to 'monaco'

* add to .gitignore and delete webpack dest before build

* increase editor height

* support more editorconfig properties

* remove empty element filter

* rename

Co-authored-by: John Olheiser <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants