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

[UI] Switch read only editors to code mirror #4924

Merged
merged 25 commits into from
Jan 2, 2024

Conversation

jpelay
Copy link
Member

@jpelay jpelay commented Dec 18, 2023

Description

We are currently using CodeMirror in the main editor, and Ace in the read-only editors, this situation is not ideal since we need to maintain two wildly different highlighting systems, therefore this PR switches all the editors to use CodeMirror.

This is the current state of the replacement:

About Parsons

The parsons exercises where deeply coupled with Ace, and the dragging code was difficult to understand. In order to switch these editors to CodeMirror it was necessary a redo of most of the code, therefore, this PR also fixes #4442 because we are now using the Sortable library, making the dragging easier. This is the parson interface now:
image

Now students can simply shuffle the lines in place, instead of dragging them from one place to the other.

@jpelay
Copy link
Member Author

jpelay commented Dec 22, 2023

Should I remove all of the code related to Ace too?

@jpelay jpelay marked this pull request as ready for review December 22, 2023 23:11
@jpelay jpelay requested a review from Felienne December 22, 2023 23:11
@Felienne
Copy link
Member

Felienne commented Dec 28, 2023

Should I remove all of the code related to Ace too?

Ow that is a tough one! One the one hand side, I would say: yes! But if it is a lot of work, maybe it is not worth it?

In any case: we want to keep the abstraction so adding another editor is easier (although I hope we will never again have to do that!!)

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Looks great @jpelay!

Just a small thing, a one line editor gets a scroll bar now (macOS, Chrome):

image

While this is not the case in the current main:
image

Since it is the first code block in the first level, it looks a bit sloppy if we leve it like this, so can you see if you can fix that?

And I'd also like a sanity check on the ts from @hasan-sh because I only skimmed it!

@@ -113,3 +160,16 @@ function fisherYatesShuffle<A>(xs: A[]) {
xs[i] = h;
}
}

export function initializeParsons() {
Copy link
Member

Choose a reason for hiding this comment

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

Really nice clean up of the Parson's code!

@@ -264,73 +264,3 @@
{% else %}
<div _="on load hedyApp.toggleDevelopersMode('load', true)"></div>
{% endif %}
<script>
// FIXME: Stop using inline JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

Haha, partly addressed with this PR? ;)

@jpelay
Copy link
Member Author

jpelay commented Dec 29, 2023

Looks great @jpelay!

Just a small thing, a one line editor gets a scroll bar now (macOS, Chrome):

image While this is not the case in the current main: image

Good catch! I'll fix it!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Looks great now!

image

Copy link
Contributor

mergify bot commented Jan 2, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@Felienne
Copy link
Member

Felienne commented Jan 2, 2024

@jpelay Do you think we need to open an issue to remind ourselves to remove Ace code? (f.e. the highlighter code that I changed in #4903)

@jpelay jpelay mentioned this pull request Jan 2, 2024
@jpelay
Copy link
Member Author

jpelay commented Jan 2, 2024

@jpelay Do you think we need to open an issue to remind ourselves to remove Ace code? (f.e. the highlighter code that I changed in #4903)

Yes, opened #4941 for that matter!

@mergify mergify bot merged commit 9af0447 into hedyorg:main Jan 2, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Jan 2, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2 participants