support non-destructive editing of content w/ legacy shortcodes #1013

alicewriteswrongs · 2022-02-16T17:47:19Z

Pre-Flight checklist

Screenshots and design review for any changes that affect layout or styling
- Desktop screenshots
- Mobile width screenshots
Testing
- Code is tested
- Changes have been manually tested

What are the relevant tickets? #1025

(Required)

What's this PR do?

This implements support for nondestructively editing markdown files which have 'legacy' shortcodes in them (these are shortcodes inserted into converted content by ocw-to-hugo to support a series of things which aren't supported in plain markdown).

How should this be manually tested?

You'll need to get some shortcode-filled markdown into one of your pages and then test out that it displays like in the little video below.

Arguments to shortcodes should be retained between saves
shortcode opening / closing state should be saved

etc

This is a little snippet that you might want to try:

Soundness and Validity
----------------------
  
{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}&nbsp; sound, satisfiable &nbsp;{{< /quiz_choice >}}
{{< quiz_choice isCorrect="false" >}}&nbsp; valid, satisfiable &nbsp;{{< /quiz_choice >}}
{{< quiz_choice isCorrect="true" >}}&nbsp; sound, valid &nbsp;{{< /quiz_choice >}}
{{< quiz_choice isCorrect="false" >}}&nbsp; valid, true &nbsp;{{< /quiz_choice >}}{{< /quiz_choices >}}
{{< quiz_solution / >}}{{< /quiz_multiple_choice >}}

it's an example of a quiz, which uses a whole bunch of shortcodes, opening and closing ones, with arguments, etc.

Screenshots (if appropriate)

Screen.Recording.2022-02-16.at.6.20.36.PM.mov

codecov-commenter · 2022-02-16T17:52:23Z

Codecov Report

Merging #1013 (c42895e) into master (cc50b15) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1013      +/-   ##
==========================================
+ Coverage   90.56%   90.67%   +0.11%     
==========================================
  Files         196      197       +1     
  Lines        7547     7627      +80     
  Branches     1318     1329      +11     
==========================================
+ Hits         6835     6916      +81     
  Misses        627      627              
+ Partials       85       84       -1

Impacted Files	Coverage Δ
static/js/lib/ckeditor/plugins/ResourceEmbed.ts	`92.18% <ø> (ø)`
static/js/lib/ckeditor/CKEditor.ts	`100.00% <100.00%> (ø)`
static/js/lib/ckeditor/plugins/LegacyShortcodes.ts	`100.00% <100.00%> (ø)`
...tic/js/lib/ckeditor/plugins/TableMarkdownSyntax.ts	`100.00% <100.00%> (ø)`
static/js/lib/ckeditor/plugins/constants.ts	`100.00% <100.00%> (ø)`
static/js/util/factories/websites.ts	`98.50% <0.00%> (+1.49%)`	⬆️

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 cc50b15...c42895e. Read the comment docs.

ChristopherChudzicki

👍 The looks good to me and is working well in local trials.

One request: I think we should add baseurl to the legacy list. No baseurls have been converted yet (hopefully will today) but even after that conversion some will remain for a while.

ChristopherChudzicki · 2022-02-17T14:02:45Z

static/js/lib/ckeditor/plugins/constants.ts

@@ -64,3 +64,25 @@ export const TABLE_ALLOWED_ATTRS: string[] = ["colspan", "rowspan"]
 * with a double quote and captures anything in between the quotes.
 */
 export const ATTRIBUTE_REGEX = /(\S+)=["']?((?:.(?!["']?\s+(?:\S+)=|\s*\/?[>"']))+.)["']?/g
+
+export const LEGACY_SHORTCODES = [


Requested Change: I think we should add baseurl to the list. Hopefully we'll remove it soon there's still some lingering baseurl issues #1015

ok cool, I was too optimistic 😃

also am I right in thinking that there's no harm in having it in the list even after the issue has been addressed via your PR?

also am I right in thinking that there's no harm in having it in the list even after the issue has been address via your PR?

yes, i agree.

If it's not there then the rule won't apply right? There shouldn't be any harm to simply including it in this list, as even if we have a need for it in the future for some reason it would be manually inserted by us and won't be touched by editors.

ChristopherChudzicki · 2022-02-17T14:04:55Z

static/js/lib/ckeditor/plugins/LegacyShortcodes.ts

+}
+
+export default class LegacyShortcodes extends Plugin {
+  static get requires(): Plugin[] {


Comment: I'm not suggesting we should necessarily worry about this now... just writing it down

I think there's something weird here... I think the Plugin[] type annotation is referring to globalThis.Plugin. https://developer.mozilla.org/en-US/docs/Web/API/Plugin , not to CKEditor plugins.

that's why it's throwing errors...I'll take a minute to see if I can fix that

weird...so I changed import Plugin from ... to import { default as CKEditorPlugin } from ... and then I get an error about being unable to use that type in a static method...I think I might just leave this off for now in the interest of time

but it would be good to fix!

ChristopherChudzicki · 2022-02-17T14:06:56Z

static/js/lib/ckeditor/plugins/LegacyShortcodes.ts

+
+              const tag = `<span ${DATA_ISCLOSING}="${isClosing}" ${
+                shortcodeArgs ?
+                  `${DATA_ARGUMENTS}="${escape(shortcodeArgs)}"` :


Question/Suggestion: I was unfamiliar with escape and VSCode highlighted it, so I looked it up on MDN. MDN says not to use it, but doesn't really explain why. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/escape Would encodeURIComponent work instead?

Edit: Google makes me think that the reason it's recommended against is that it occasionally produces invalid URLs. Since we're not using it for that, probably not a concern? But if something not deprecated works, that seems better.

I think we can use encoreURIComponent, I'll swap that in and make sure the tests pass real quick. The reason to do it is just that if it's not escaped then the text w/in the arguments to a shortcode can cause issues correctly finding the boundaries of the HTML tags later on and things get all messed up. When I was hacking on it I actually used btoa and atob to be 100% sure I wasn't going to get any characters I didn't expect in the arguments string, haha

Yea, it seems that escape doesn't support "higher order unicode characters," and the only suggestion I see as a replacement as well is encode/decodeUriComponent

yep, I already made the change since all the tests pass with it

gumaerc

I'm approving this, as it tested out well for me locally. I think the things @ChristopherChudzicki noted should be checked out still though. The escape thing isn't critical as it's not going away anytime soon, but might be better to use encodeUriComponent.

gumaerc · 2022-02-17T14:56:31Z

static/js/lib/ckeditor/plugins/LegacyShortcodes.ts

+
+              const tag = `<span ${DATA_ISCLOSING}="${isClosing}" ${
+                shortcodeArgs ?
+                  `${DATA_ARGUMENTS}="${escape(shortcodeArgs)}"` :


Yea, it seems that escape doesn't support "higher order unicode characters," and the only suggestion I see as a replacement as well is encode/decodeUriComponent

gumaerc · 2022-02-17T15:09:04Z

static/js/lib/ckeditor/plugins/constants.ts

@@ -64,3 +64,25 @@ export const TABLE_ALLOWED_ATTRS: string[] = ["colspan", "rowspan"]
 * with a double quote and captures anything in between the quotes.
 */
 export const ATTRIBUTE_REGEX = /(\S+)=["']?((?:.(?!["']?\s+(?:\S+)=|\s*\/?[>"']))+.)["']?/g
+
+export const LEGACY_SHORTCODES = [


If it's not there then the rule won't apply right? There shouldn't be any harm to simply including it in this list, as even if we have a need for it in the future for some reason it would be manually inserted by us and won't be touched by editors.

This implements support for nondestructively editing markdown files which have 'legacy' shortcodes in them (these are shortcodes inserted into converted content by ocw-to-hugo to support a series of things which aren't supported in plain markdown). A manifest of these shortcodes is declared in `static/js/lib/ckeditor/plugins/constants.ts`, and support for them is implemented in a new CKEditor plugin called `LegacyShortcodes`. This will enable non-destructive editing of markdown content which contains these shortcodes, fixing an issue we were seeing where raw shortcodes that were unknown to CKEditor were being escaped and the showing up in the rendered HTML. This fixes this by adding a node type to CKEditor for each one, and adding basical logic for preserving any arguments to the shortcodes. Note that this change does NOT in any way way enable _editing these shortcodes themselves_. Instead, they are made obvious in the editor with a basic display, but it is up to the content editors to not delete them or anything like that (and right now they could only be added to a file manually in the Django admin or shell). pr #1013

alicewriteswrongs force-pushed the ap/legacy-shortcodes branch from 54ae91c to 1767922 Compare February 16, 2022 23:13

alicewriteswrongs changed the title ~~[WIP] support non-destructive editing of content w/ legacy shortcodes~~ support non-destructive editing of content w/ legacy shortcodes Feb 16, 2022

ChristopherChudzicki self-requested a review February 17, 2022 13:05

ChristopherChudzicki requested changes Feb 17, 2022

View reviewed changes

gumaerc approved these changes Feb 17, 2022

View reviewed changes

ChristopherChudzicki approved these changes Feb 17, 2022

View reviewed changes

alicewriteswrongs force-pushed the ap/legacy-shortcodes branch from c42895e to 07290e8 Compare February 17, 2022 15:34

alicewriteswrongs merged commit 07290e8 into master Feb 17, 2022

alicewriteswrongs deleted the ap/legacy-shortcodes branch February 17, 2022 15:35

alicewriteswrongs temporarily deployed to ocw-studio-ci February 17, 2022 15:47 Inactive

This was referenced Feb 17, 2022

Legacy Shortocdes: Support Editing and Fix Data #1025

Closed

Data migration: Finish baseurl conversion #1015

Closed

ChristopherChudzicki mentioned this pull request Apr 13, 2022

[markdown cleanup] fix superscript/subscript escaping issues #1241

Merged

3 tasks

ChristopherChudzicki mentioned this pull request May 11, 2022

Saving pages with image galleries in studio messes them up #1317

Closed

ChristopherChudzicki mentioned this pull request Nov 1, 2023

Legacy Shortcodes with Separate Opener/Closer can be corrupted #2016

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support non-destructive editing of content w/ legacy shortcodes #1013

support non-destructive editing of content w/ legacy shortcodes #1013

alicewriteswrongs commented Feb 16, 2022 •

edited by ChristopherChudzicki

Loading

codecov-commenter commented Feb 16, 2022 •

edited

Loading

ChristopherChudzicki left a comment

ChristopherChudzicki Feb 17, 2022 •

edited

Loading

alicewriteswrongs Feb 17, 2022

alicewriteswrongs Feb 17, 2022 •

edited

Loading

ChristopherChudzicki Feb 17, 2022

gumaerc Feb 17, 2022

ChristopherChudzicki Feb 17, 2022

alicewriteswrongs Feb 17, 2022

alicewriteswrongs Feb 17, 2022

alicewriteswrongs Feb 17, 2022

ChristopherChudzicki Feb 17, 2022 •

edited

Loading

alicewriteswrongs Feb 17, 2022

gumaerc Feb 17, 2022

alicewriteswrongs Feb 17, 2022

gumaerc left a comment

gumaerc Feb 17, 2022

gumaerc Feb 17, 2022

support non-destructive editing of content w/ legacy shortcodes #1013

support non-destructive editing of content w/ legacy shortcodes #1013

Conversation

alicewriteswrongs commented Feb 16, 2022 • edited by ChristopherChudzicki Loading

Pre-Flight checklist

What are the relevant tickets? #1025

What's this PR do?

How should this be manually tested?

Screenshots (if appropriate)

codecov-commenter commented Feb 16, 2022 • edited Loading

Codecov Report

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

ChristopherChudzicki Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumaerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Feb 16, 2022 •

edited by ChristopherChudzicki

Loading

codecov-commenter commented Feb 16, 2022 •

edited

Loading

ChristopherChudzicki Feb 17, 2022 •

edited

Loading

alicewriteswrongs Feb 17, 2022 •

edited

Loading

ChristopherChudzicki Feb 17, 2022 •

edited

Loading