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

Blog feed fixes #370 #407

Merged
merged 2 commits into from
Feb 11, 2018
Merged

Blog feed fixes #370 #407

merged 2 commits into from
Feb 11, 2018

Conversation

tom-auger
Copy link
Contributor

@tom-auger tom-auger commented Jan 21, 2018

Motivation

Fix bug detailed in issue #370

Closes #370

Test Plan

The test docusaurus website has a blog post with a description containing HTML.

  1. Manually check that blog posts/documentation pages still correctly render markdown as HTML.
  2. Manually check the rss and atom feeds (http://localhost:3000/blog/feed.xml and http://localhost:3000/blog/atom.xml) now have the description in HTML rather than markdown.

Related PRs

None

@JoelMarcey
Copy link
Contributor

@tom-auger Thanks for this pull request Tom!

I am bringing @hramos into this review since he brought Remarkable into our codebase and he also has an outstanding issue of #403 which this PR may resolve.

@JoelMarcey
Copy link
Contributor

@tom-auger Btw, what was the impetus into splitting the current Remarkable.js files into two files, Remarkable.js and renderMarkdown.js.

I think that might be ok and maybe even the right thing to do, but wanted to get your input on your motivation there.

@tom-auger
Copy link
Contributor Author

@JoelMarcey

The main reason I split it up was that the Remarkable Markdown class only needs to be instantiated once and can then be reused. The original code created the Markdown object inside renderMarkdown in the Remarkable React component. This meant each individual Remarkable component created its own instance of Markdown. Instead by putting it in a separate module the Markdown class is instantiated once and then reused between Remarkable components. I also felt it was more natural having the rendering of markdown as a function rather than a React component.

However, having done this there are now two components for rendering markdown: MarkdownBlock and Remarkable. If we want to keep my refactor then they should probably be merged into one (perhaps just MarkdownBlock). In any case, I think it would be nicer if there's only one React component for rendering markdown.

I'm also unsure about my change in feed.js. It might be more idiomatic to wrap the content in a MarkdownBlock and get React to render it, e.g.

ReactDomServer.RenderToStaticMarkup(<MarkdownBlock>{content}</MarkdownBlock>).

On the other hand using React to essentially wrap and unwrap some html is perhaps a bit overkill.

Apologies for the long reply! I'm happy to make changes, let me know what you think.

@danphamus304
Copy link

Pull request

@JoelMarcey
Copy link
Contributor

@tom-auger Hi. I like your rationale. I am good with this PR.

Re: your change in feed.js, thinking about it a bit, what you did seems quite reasonable to me. If someone comes up with a good reason to change it, we can do that in another PR. What you have now is important as it fixes a pretty good bug.

@JoelMarcey JoelMarcey merged commit 2d7274f into facebook:master Feb 11, 2018
@tom-auger tom-auger deleted the fix-blog-feed branch February 12, 2018 18:29
@tom-auger
Copy link
Contributor Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants