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

Line breaks in the markdown docs are translated as <br /> tags erroneously. #384

Open
thenickcox opened this issue Mar 30, 2015 · 9 comments

Comments

@thenickcox
Copy link

This has been bothering me about the docs for a while. I recently had a documentation PR merged in Marionette core, and part of that removed the line breaks in the docs because they were being converted to <br /> tags. But I was browsing the Markdown docs today, and a Markdown parser should only insert a <br /> tag if there are two line breaks. This is not the case on these docs; each single line break in the docs is parsed as a <br /> tag. (To verify, check the compiled HTML)

I did some digging and thought maybe it was an issue with marked, the markdown parsing library used to compile the docs here. So I made a Codepen to test some markdown with a line break in it, and it correctly renders as a space.

My conclusion at this point is that it's something in the compile-docs.js task. But nothing immediately jumps out to me as being at fault.

I'm filing this issue because I'm working on a PR for some docs, and I had been removing the line breaks so that they don't render as <br /> tags, but I'm now convinced that's not a great solution, because it makes them harder to read.

@thenickcox thenickcox changed the title Line breaks in the markdown docs are translated as <br /> tags erroneously. Line breaks in the markdown docs are translated as <br /> tags erroneously. Mar 30, 2015
@samccone
Copy link
Member

@thenickcox The only other piece of the process would be our fork of docco that @peterblazejewicz did.

@peterblazejewicz any thoughts?

@thenickcox
Copy link
Author

Now that we're talking about it, there is a call in the getDescription method in the compile-docs.js that specifically replaces <br> tags, which shouldn't be necessary with marked's default output. It occurred to me to do the same thing in the compileContent call, but that feels hacky.

@samccone
Copy link
Member

hmm open to ideas of fixes that you think can make it better, but yeah all of the docs require a bit of massaging to get "right"

@thenickcox
Copy link
Author

I mean, if the core team thought that would be an acceptable fix, far be it from me to say otherwise. Let me know if you'd like me to submit a PR with that same function call (or abstracted to avoid duplication), and I'll go ahead with that.

@samccone
Copy link
Member

@thenickcox that would be awesome!

@thenickcox
Copy link
Author

Great. The only downside I could see to that is that any intentional <br> tag in the docs would get thrown out. We could add a check for a class like "no-replace" or something, but I don't think I've ever intentionally used a <br> in markdown.

@peterblazejewicz
Copy link
Member

@samccone @thenickcox do compile-docs download only release-based content? So 2.4.1 tagged release content is downloaded, not the current docs from master - if that is true, the changes (line breaks and tweeks) from linked PR are not yet within compiled docs - I've check paragraphs, e.g.:
marionettejs/backbone.marionette@18d3227#diff-65e45658842b27540d8f45490a59c2fbL120

@thenickcox
Copy link
Author

@thenickcox
Copy link
Author

Okay. Thanks to @peterblazejewicz's fix, I should be able to take a look at this tonight.

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

Successfully merging a pull request may close this issue.

3 participants