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

Render teacher_markdown clientside #33254

Merged
merged 5 commits into from Mar 9, 2020
Merged

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Feb 21, 2020

So we can render it with our SafeMarkdown component.

Prerequisite to opening this field up for translations.

Because this is switching the renderer from redcarpet to remark, it also requires content updates for many of the affected levels; that work is being tracked here.

Links

Testing story

As with other renderer switches, the testing pattern for this is to:

  • Output all affected markdown to a markdown.json file
  • Render all markdown with Redcarpet, output to redcarpet.json
  • Render all markdown with Remark, output to remark.json
  • Use https://github.com/Hamms/html-equivalency to compare redcarpet.json and remark.json and identify the levels for which there is an html change

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@Hamms Hamms marked this pull request as ready for review February 24, 2020 22:01
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Code changes look good, but can you please remind me, why is it important to switch to client-side rendering via SafeMarkdown? is the important part to use remark rather than redcarpet, and can we not render remark server-side?

dashboard/app/views/levels/_teacher_markdown.html.haml Outdated Show resolved Hide resolved
@@ -542,6 +542,7 @@ describe('entry tests', () => {
'levels/_bubble_choice':
'./src/sites/studio/pages/levels/_bubble_choice.js',
'levels/_content': './src/sites/studio/pages/levels/_content.js',
'levels/_teacher_markdown': './src/sites/studio/pages/levels/_teacher_markdown.js',
Copy link
Member

Choose a reason for hiding this comment

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

I hate to make style comments now that we have prettier, but can you please preserve the alphabetical order?

@@ -10,23 +10,6 @@
border: 1px dashed black;
padding: 10px;
}
.teacher_markdown {
Copy link
Member

Choose a reason for hiding this comment

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

nice cleanup!

Co-Authored-By: Dave Bailey <davidsbailey@users.noreply.github.com>
@Hamms
Copy link
Contributor Author

Hamms commented Feb 24, 2020

The important part is to use a renderer that can detect and remove "unsafe" markup; we currently have a solution for this using remark and hast-util-sanitize and do not have a solution for redcarpet, nor do we have a way to render remark serverside.

@davidsbailey
Copy link
Member

The important part is to use a renderer that can detect and remove "unsafe" markup; we currently have a solution for this using remark and hast-util-sanitize and do not have a solution for redcarpet, nor do we have a way to render remark serverside.

Thanks @Hamms . what categories of content need detection/removal of "unsafe" markup?

@Hamms
Copy link
Contributor Author

Hamms commented Feb 24, 2020

Any content that is externally sourced; in this case, sourced from translators.

@Hamms
Copy link
Contributor Author

Hamms commented Mar 9, 2020

All affected levels have been updated (https://docs.google.com/spreadsheets/d/1p-KP5RMQn3d-aNuagCCyGKyUfC7L8893gPoT2e3fCmA/edit#gid=153934544), and I've confirmed that no new inconsistencies were added in the last two weeks, so this is now safe to merge!

@Hamms Hamms merged commit 4043901 into staging Mar 9, 2020
@Hamms Hamms deleted the render-teacher-markdown-clientside branch March 9, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants