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 dangeroulsySetInnerHtml in NonMarkdownInstructions with UnsafeRenderedMarkdown #27749

Merged

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Mar 28, 2019

I thought we actually didn't use this component for rendering instructions anymore - just for rendering the title in things like anigif detail views - but it turns out we do still use it to display instructions in netsim, so I can't actually remove it entirely :(

@Hamms Hamms requested a review from islemaster March 28, 2019 19:30
Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

I wonder if anyone would complain if we switched to the markdown instructions in NetSim.

@Hamms
Copy link
Contributor Author

Hamms commented Mar 28, 2019

I'd be in favor of that.

Specifically, what we do now is use the MarkdownInstructions component for any netsim levels that have Long Instructions, and this component for any levels that don't (including both levels that only have Short Instructions, and any theoretical levels that have neither).

We also (for what I assume are historical reasons) use the NonMarkdownInstructions component to render the level title for things like the AniGif detail view.

It would probably be pretty simple to use MarkdownInstructions for levels that have either set of instructions, and render the title element as a standalone for those views that need it.

@Hamms
Copy link
Contributor Author

Hamms commented Mar 28, 2019

Something along the lines of #27752 should be what we want

@islemaster
Copy link
Contributor

Cool. I'm fine with either approach, I assume we'd want curriculum input on the latter but maybe worth getting it.

@Hamms Hamms merged commit 511569d into staging Mar 29, 2019
@Hamms Hamms deleted the replace-dangerouslySetInnerHtml-in-NonMarkdownInstructions branch March 29, 2019 23:54
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