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

fix: make referenced links work with code block tabs #1249

Conversation

fiennyangeln
Copy link
Contributor

Motivation

  • The reason for this bug that the previous Codeblock implementation separates
    the tabs, the markdown before tabs, and the markdown after tabs into
    separate Remarkable component, thus they don't share information
    regarding the reference link
  • To solve this, change the Doc implementation so that one Doc
    have only one Remarkable component by transforming the codeblock
    into html string and add it as part of the markdown, letting the
    Remarkable take care of the html string
  • However, this approach made us need to ensure that there is no
    newline in the codetab, otherwise, the formatting inside the
    code will be broken. Thus, I replace every newline inside the
    code tag with a br tag

Fix #1215

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Create a referenced link with the reference located separately (one above the Codeblock tab, one below the Codeblock tab), e.g:

  1. Add [You can use numbers for reference-style link definitions][1] anywhere above the Codeblock tab
  2. Add [1]: http://slashdot.org anywhere below the Codeblock tab

Related PRs

None

- The reason is that the previous Codeblock implementation separates
the tabs, the markdown before tabs, and the markdown after tabs into
separate Remarkable component, thus they don't share information
regarding the reference link
- To solve this, change the Doc implementation so that one Doc
have only one Remarkable component by transforming the codeblock
into html string and add it as part of the markdown, letting the
Remarkable take care of the html string
- However, this approach made us need to ensure that there is no
newline in the codetab, otherwise, the formatting inside the
code will be broken. Thus, I replace every newline inside the
code tag with a br tag

Fix facebook#1215
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 26, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 9f4a528

https://deploy-preview-1249--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit afdfb89

https://deploy-preview-1249--docusaurus-preview.netlify.com

@yangshun yangshun changed the title Fix bug Code block tabs broke the referenced links fix: make referenced links work with code block tabs Feb 26, 2019
@yangshun yangshun merged commit a1d36be into facebook:master Feb 26, 2019
@yangshun
Copy link
Contributor

Thanks for the quick fix @fiennyangeln! Now that I think about it, I think this feature should be implemented as a markdown plugin that has access to the markdown AST instead of us doing our own parsing so that we don't run into similar problems again. I suspect we might have other problems surfaced in future due to this approach as there are so many Markdown syntax that we have to beware of.

If you have time and are up for a challenge you can try converting this feature into a remarkable plugin 😄

@JoelMarcey
Copy link
Contributor

I think this feature should be implemented as a markdown plugin that has access to the markdown AST instead of us doing our own parsing so that we don't run into similar problems again. I suspect we might have other problems surfaced in future due to this approach as there are so many Markdown syntax that we have to beware of.

@yangshun Are you thinking this for v2? Or a major update for v1?

@yangshun
Copy link
Contributor

In v2 we'll definitely write this as a Markdown plugin, but I think that logic can be reused, so if we write it in v1 now we can also use it in v2, and vice versa.

@fiennyangeln
Copy link
Contributor Author

I saw remarkable-codegroup which does similar thing, however, it requires jQuery and bootstrap3

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.

None yet

5 participants