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(v2): fix code block title parsing, support multiple metastring attributes #4541

Merged
merged 5 commits into from
Apr 14, 2021

Conversation

duanwilliam
Copy link
Contributor

Motivation

In its current form, the docusaurus-theme-classic CodeBlock title captures everything from its opening delimiter to the last double quote in the line. This means other code block metadata must either use a delimiter other than double quotes, or be placed before the title=" delimiter. Changing it to only capture until the next double quote would allow greater flexibility in code block metadata.

Doing this change would prevent double quotes from being parts of code block titles. A simple and self-evident solution, implemented in this PR as well, is in allowing the title to be delimited by single quotes as well would accommodate for these cases. It should be noted, however, that code block titles containing both single and double quotes would still have no alternative. Other ideas on how to approach this issue would be beneficial.

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

I swizzled the classic theme CodeBlock component and made the changes on one of my projects using Docusaurus before making this.

```. dead='' beef="" title="foo'bar"{1} foo="" bar=''
```. dead='' beef="" title="foo'bar"{1} foo="" bar=''
```

```md title='foo"bar'foo="" bar=''
```md title='foo"bar'foo="" bar=''
```

```js title='mismatched"
```js title='mismatched"
```

```. title="mismatched'
```. title="mismatched'
```

image

Related PRs

The code block docs do not mention that the title must currently be enclosed by double quotes, nor that it captures from the opening double quote to the last double quote in the line.

I can update the docs upon request or if/when this gets approved.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 1, 2021
@github-actions
Copy link

github-actions bot commented Apr 1, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 50
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4541--docusaurus-2.netlify.app/

@slorber
Copy link
Collaborator

slorber commented Apr 9, 2021

Thanks

I know we don't have much tests in the theme currently but our goel is to add tests.

Can you please extract a parseCodeBlockTitle function, add tests for basic title parsing + the edge-case that you are trying to solve?

@slorber slorber marked this pull request as draft April 13, 2021 10:42
@netlify
Copy link

netlify bot commented Apr 13, 2021

[V2]

Built with commit aed1216

https://deploy-preview-4541--docusaurus-2.netlify.app

@duanwilliam
Copy link
Contributor Author

duanwilliam commented Apr 13, 2021

ah sorry was kinda busy.
I don't know if I necessarily understand what you intended, but I extracted parseCodeBlockTitle to a separate file in the CodeBlock directory and created tests for it.
I also couldn't quite find what the standard practice was for making tests.
Let me know what needs to be changed to make it more consistent with the rest of the codebase.

also, just as a note, swizzling the resultant CodeBlock swizzles the __tests__ directory as well; is this intended behavior?

@duanwilliam duanwilliam marked this pull request as ready for review April 13, 2021 22:33
@slorber
Copy link
Collaborator

slorber commented Apr 14, 2021

Thanks, this is what I had in mind.
Just moved the code to theme-common as we'll likely share it between multiple themes in the future.

Will take a look for swizzle and test folder, thanks.

@netlify
Copy link

netlify bot commented Apr 14, 2021

[V1]

Built with commit aed1216

https://deploy-preview-4541--docusaurus-1.netlify.app

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Apr 14, 2021
@slorber slorber changed the title feat(v2): improve classic theme code block title parsing fix(v2): fix code block title parsing, support multiple metastring attributes Apr 14, 2021
@slorber slorber merged commit 1abadba into facebook:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants