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 valid React for Blockly XML in <SafeMarkdown/> #42415

Merged
merged 5 commits into from
Sep 22, 2021

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Sep 9, 2021

This change updates our <SafeMarkdown/> component to render Blockly XML in a way that is valid React. We do this by wrapping each Blockly element in a custom component and using the components option in rehype-react.

Since Blockly elements aren't valid React (e.g., <xml> isn't a valid React component), we were previously seeing warnings any time Blockly blocks were rendered by our instructions panel:
image (3) (1)

This would also cause unit tests to fail for the same reason (we fail unit tests when console.warn is called), so we no longer have to skip or allow warnings in InstructionsCSFTest.js.

Links

  • STAR-1680 - Unskip InstructionsCSFTest
  • STAR-1708 - <InstructionsCSF/> logs warnings for instructions with embedded Blockly blocks

Testing story

Updates unit tests.

@maddiedierker maddiedierker requested review from a team September 10, 2021 20:17
@maddiedierker maddiedierker changed the title React allow xml instructions Render valid React for Blockly XML in <SafeMarkdown/> Sep 10, 2021
Copy link

@epeach epeach left a comment

Choose a reason for hiding this comment

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

Do we know how this react component is read within the TTS system?

@dju90
Copy link
Contributor

dju90 commented Sep 10, 2021

fwiw Blockly blocks are not currently read by our TTS (neither Acapela nor Immersive Reader)

@maddiedierker
Copy link
Contributor Author

Do we know how this react component is read within the TTS system?

along with bryan's point, the resulting HTML output is nearly identical, so i don't think this change should affect our TTS, but i don't know the best way to double-check locally. i used our TTS feature on this level on both my feature branch and staging and there was no difference. i don't have an API key for immersive reader locally, so i'm unable to test that. i think i remember that our TTS files are generated at some point, which is why i'm not sure my testing method was actually the correct way to go about it.

@maddiedierker
Copy link
Contributor Author

@epeach PTAL! let me know if there's a better way to test TTS with this change as well

@maddiedierker maddiedierker merged commit 80ac970 into staging Sep 22, 2021
@maddiedierker maddiedierker deleted the react-allow-xml-instructions branch September 22, 2021 17:01
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

3 participants