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

Replace Ace Editor with CodeMirror 6 #4970

Merged
merged 13 commits into from
Oct 30, 2023
Merged

Replace Ace Editor with CodeMirror 6 #4970

merged 13 commits into from
Oct 30, 2023

Conversation

niknetniko
Copy link
Member

@niknetniko niknetniko commented Sep 15, 2023

This reverts commit 98c93fe, and thus re-introduces #4933.


This pull request replaces the Ace Editor with CodeMirror (which is a commonly-used text editing component for the web).

Feature-wise, codemirror is mostly equivalent. There are some changes:

  • autocomplete is now shown by default instead of only on ctrl + space
  • ace had linting for Javascript, but this caused confusion (ACE Editor does not support latest JavaScript features #4585). It is now disabled (also for other languages)
  • the syntax highlighting is slightly different, but now equivalent to rouge which is used on all other places on Dodona
  • the font size is slightly larger

There are a number of other benefits:

  • Uses javascript packages instead of ruby gems which rely on sprockets magic
  • Ready for use within WebComponents

On the other hand, it does need a bit more code to:

  • Set up the language support (since the set of first-party language modules is smaller)
  • Set up the theme. It applies the Rouge CSS classes to the code, so it uses the same theme. However, sometimes CodeMirror and Rouge don't agree on what a particular piece of code is, so I foresee that we might need to tweak the theme a bit sometimes.

Closes #4585 as a side effect.

@niknetniko niknetniko added the deploy mestra Request a deployment on mestra label Sep 15, 2023
@niknetniko niknetniko temporarily deployed to mestra September 15, 2023 13:55 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Sep 15, 2023
@niknetniko niknetniko added the deploy naos Request a deployment on naos label Sep 18, 2023
@niknetniko niknetniko temporarily deployed to naos September 18, 2023 12:16 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Sep 18, 2023
@niknetniko
Copy link
Member Author

When testing, this seems to work on both Mestra and Naos, so I am not entirely sure why it didn't work in production.

@jorg-vr
Copy link
Contributor

jorg-vr commented Sep 20, 2023

The potential problem might be dynamic loading of javascript

If codemirror loads certain packages only if the language requires them, it needs to use the correct paths.
The assets paths are still managed by rails sprockets, so the issue might be there.

I remember you also saying that on the broken pages there was a javascript fetch error

@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Sep 27, 2023
@jorg-vr jorg-vr temporarily deployed to naos September 27, 2023 12:08 — with GitHub Actions Inactive
@jorg-vr jorg-vr self-assigned this Oct 26, 2023
@jorg-vr jorg-vr added deploy naos Request a deployment on naos and removed deploy naos Request a deployment on naos labels Oct 26, 2023
@jorg-vr jorg-vr temporarily deployed to naos October 26, 2023 15:02 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Oct 26, 2023
@jorg-vr
Copy link
Contributor

jorg-vr commented Oct 26, 2023

confirmed bug to be related to #5082
image

So fix might have fixed this

@niknetniko niknetniko added the deploy naos Request a deployment on naos label Oct 26, 2023
@niknetniko niknetniko temporarily deployed to naos October 26, 2023 15:12 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Oct 26, 2023
@niknetniko niknetniko marked this pull request as ready for review October 26, 2023 15:17
@niknetniko niknetniko requested review from jorg-vr and a team as code owners October 26, 2023 15:17
@niknetniko niknetniko requested review from bmesuere and removed request for a team October 26, 2023 15:17
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

  • Autocomplete doesn't seem to work. E.g. html autocomplete in the docs https://codemirror.net/examples/autocompletion/ vs on https://naos.dodona.be/nl/activities/1412931994/

  • Font size is a little bigger, but not sure if this is a problem.

  • We seem to miss the highlighting of functions compared to ace.

  • Ace had basic linting (in javascript), this is missing now. Not sure if this is a problem (since the linting messages could be different from the ones in the judge and thus confusing), but maybe mention it in the PR.

  • I also think the python indentation is different (or at least not very smart). In a solution that used 4 spaces, it added just 2 when adding a new line.

  • The active line isn't marked anymore.

app/assets/javascripts/editor.ts Outdated Show resolved Hide resolved
@bmesuere bmesuere changed the title Replace Ace Editor with CodeMirror II Replace Ace Editor with CodeMirror 6 Oct 27, 2023
@bmesuere bmesuere added the enhancement A change that isn't substantial enough to be called a feature label Oct 27, 2023
@jorg-vr jorg-vr marked this pull request as draft October 27, 2023 12:51
@jorg-vr jorg-vr marked this pull request as ready for review October 27, 2023 15:41
@jorg-vr
Copy link
Contributor

jorg-vr commented Oct 27, 2023

Changes:

  • autocomplete now works and is applied by default
  • font size > I prefer the larger size, but did make the line numbers a bit smaller
  • The python lexer does not indicate builtins in the same way as ace, so we canot color them specifically. For the fans of a bit more color, I did change the highlighting for functions to yellow in both the editor and rouge (to keep it consistent)
  • Added a comment that linting is removed in pr
  • I made the default indent 4 spaces. This is the most common for python and python is our most used language. I could not find a smart detection algorithm in the editor. We could write such an algorithm ourselves, or provide a different default for each language if desired
  • I reintroduced active line marking. The color has to be a partially transparant version of the slection color to work well with selected code

@jorg-vr jorg-vr requested a review from bmesuere October 27, 2023 15:48
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Oct 27, 2023
@jorg-vr jorg-vr temporarily deployed to naos October 27, 2023 15:49 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Oct 27, 2023
@jorg-vr jorg-vr merged commit 1853849 into main Oct 30, 2023
13 of 15 checks passed
@jorg-vr jorg-vr deleted the enhance/codemirror branch October 30, 2023 07:52
@jorg-vr jorg-vr temporarily deployed to naos October 30, 2023 07:52 — with GitHub Actions Inactive
@jorg-vr jorg-vr temporarily deployed to production October 30, 2023 07:58 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACE Editor does not support latest JavaScript features
3 participants