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 Prismjs and manual code editing system with CodeMirror #888

Merged
merged 16 commits into from
Oct 25, 2022

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Sep 13, 2022

This PR replaces the Prism code highlighting and manual code editing system with CodeMirror. This significantly reduces the dependency count and simplifies our code. Closes #886 due to irrelevance. Naturally fixes #858, fixes #774, fixes #813, fixes #811 by using the CodeMirror theme.

This will currently block work for #851.

@queengooborg queengooborg mentioned this pull request Sep 13, 2022
12 tasks
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner
Copy link
Contributor

caugner commented Sep 22, 2022

@queengooborg As this PR changes how the interactive examples look, could you please add Before/After screenshots for each of the 4 editor types? 🙏

@queengooborg
Copy link
Collaborator Author

queengooborg commented Sep 22, 2022

Screenshots now attached! This actually only affects the CSS examples (all of the other editors use CodeMirror)!

Before:
Screen Shot 2022-09-22 at 04 16 22
(Note that the black text on dark theme is due to a CSS issue that will be resolved in another PR if this one doesn't land.)

After:
Screen Shot 2022-09-22 at 04 14 42

@caugner
Copy link
Contributor

caugner commented Sep 22, 2022

I think the reason for using Prismjs here was to make the interactive examples look like the code samples (which will continue to use Prism): https://developer.mozilla.org/en-US/docs/Web/CSS/animation#applying_multiple_animations

Not really sure how to solve this. 🤔

@queengooborg
Copy link
Collaborator Author

Sadly, from what I noticed, it seemed as though Prismjs never actually ran on the interactive examples -- accessing a random CSS page with an interactive example, the text is a uniform color: https://developer.mozilla.org/en-US/docs/Web/CSS/transition

Screen Shot 2022-09-23 at 02 48 03

Additionally, once I got it working again, I ran into some other unfortunate issues that deteriorate the user experience, such as: if we perform syntax highlighting while typing, the user loses their undo state and it's quite hard to retain the text cursor position when the HTML changes, but if we don't, the user will end up getting colors bleeding over into parts it shouldn't (if that makes any sense).

Personally, I think that the benefits to using CodeMirror everywhere outweigh the drawbacks, especially when considering that this same issue applies to all of the existing CodeMirror editors.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@queengooborg
Copy link
Collaborator Author

@schalkneethling @caugner May I get a review on this soon?

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one possible nit.

editor/js/editor-libs/events.js Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

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