-
Notifications
You must be signed in to change notification settings - Fork 29
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 bootstrap-pagedown and ace markdown editors with ToastUi #2242
Replace bootstrap-pagedown and ace markdown editors with ToastUi #2242
Conversation
fd7bc8a
to
22b0a1b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2242 +/- ##
==========================================
+ Coverage 69.26% 69.30% +0.04%
==========================================
Files 197 197
Lines 6228 6230 +2
==========================================
+ Hits 4314 4318 +4
+ Misses 1914 1912 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I am really happy and thankful for the great work you've done here to replace the editor, @JuliaCasamitjana -- I absolutely appreciate it! 👏
Below is a first round of review, mostly focussing on the functionality. Some things might not be that easy and can have a lower priority. Still, some of the errors might be related to our integration and not only focused on bugs in the editor.
Besides that, I also want to take the chance to reply to your decision / comments:
- Not using the hacky workaround for the preview is fine; I've seen that you've applied the same style and otherwise made the preview as good as possible. For our use case, it is totally fine to wait for upstream fixes.
- The URL image insertion is fine and I would like to keep that. The broken markdown rendering you've seen is based on our Content Security Policy (CSP), limiting allowed images to a few domains. You can configure those yourself in
config/content_security_policy.yml
. By default, we only allow the following:codeocean/config/content_security_policy.yml.example
Lines 8 to 12 in 3e956bb
img_src: - https://s3.xopic.de - https://*.s3.xopic.de - https://s3.openhpicloud.de - https://*.s3.openhpicloud.de
A real world URL to test with that should work is https://openhpi-public.s3.openhpicloud.de/courses/5SOxFgxluGNuvuuAdVwDpH/rtfiles/4Qrt5T4mzPnwSqis3W00DM/a223.png
Many thanks again for all the effort you've put into this PR already and the extra mile (I've happily seen the theming support or your changes to the FormBuilder and all those "minor" details that sum up) 👍.
5af3485
to
94b0773
Compare
I've realised there is one functionality we added in Xikolo that I did not include here. It's a custom implementation to expand the editor to fit all contents: https://lab.xikolo.de/xikolo/web/-/merge_requests/4278 Is this something you would also like to have here? |
Ah, that's nice! Yes, if you can easily add it by reusing the existing code, I would be glad to get this custom add-on, too 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just testing the new editor again and am really satisfied with the changes and the new look-and-feel it provides 🚀.
94b0773
to
17ebb1d
Compare
3d6263c
to
c1e755d
Compare
c1e755d
to
e131e81
Compare
4291e45
to
b628ab6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 👏 🥳
I've just tested the current changes a final time and am really satisfied with the result. This PR is a major leap forward for CodeOcean, since we are dropping two custom editor solutions and an outdated library. Many thanks for your endurance to tackle all those bugs I've discovered during review and the extra mile you went ahead. There are so many nice additions I see, starting from a solid code base 🏗️ to theme support 🎨.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great job, @JuliaCasamitjana! This is a big achievement. Also, thanks to you @MrSerth for all the support.
Replace all usages of pagedown-bootstrap editor with the new editor. Add styles to ensure the editor preview matches the final output.
The default ToastUI image insertion feature includes both URL and file upload options. However, file upload functionality isn't supported in the application. This commit addresses the issue by implementing custom code to hide the file upload button while preserving the URL insertion option.
The term 'Pagedown' was originally associated with the pagedown-bootstrap library, which is no longer in use.
Drop unused code related to the Ace markdown editor.
The editor will now have a default height of 300px but a button will let the user expand the editor. It will expand it to fit all content (or up to 400px if the content was not exceeding 300px). In the expanded mode the editor will keep growing as the user types more content.
Manually inserting a codeblock adding three backticks and hitting enter is not functioning in the ToastUI editor due to an existing bug in the library. This commit implements a workaround to address the issue.
b628ab6
to
f452e26
Compare
What does this change?
This PR integrates the ToastUi library and uses it in all forms where markdown editors are required. It replaces the previously used
boostrap-pagedown
editor (used for exercises, proxy exercises and tips descriptions) and the Ace markdown editor (used in the help field from the execution environment form). It also removes all related code from the dropped editors.Note: There are two open bugs affecting the ToastUI preview:
referenceDefinition
to support such links breaks the preview functionality. It has to be disabled.Current behaviour with such bugs:
Decisions / Choices I made
markdown-it
) and replacing the contents of the toastUi preview accordingly on the fly. I did not include this workaround, but implementing a similar solution here wouldn't require significant additional effort if we prefer to adopt this somewhat hacky approach.