-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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(theme): make Prism code block language / additionalLanguages case insensitive #9183
Conversation
Signed-off-by: Sujal Gupta <sujalgupta6100@gmail.com>
Signed-off-by: Sujal Gupta <sujalgupta6100@gmail.com>
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
You can keep phP
or something like that in the additional languages of this PR so that we can make PHP work in the preview and be able to validate the PR without running local modifications. We are tracking screenshots and do visual regression tests of our pages now so that's a good way for us to ensure that this feature keeps working over time if we actually use it on our site.
Currently there's no highlighting there:
https://deploy-preview-9183--docusaurus-2.netlify.app/tests/pages/code-block-tests/#code-block-withwithout-the-good-prism-language-case-in-additionallanguages-tests
@@ -27,6 +27,21 @@ See: | |||
- https://github.com/facebook/docusaurus/pull/3749 | |||
- https://github.com/facebook/docusaurus/pull/6177 | |||
|
|||
## Code block with/without the good prism language case in additionalLanguages[] tests | |||
|
|||
```php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be nice to add 2 other cases like PHP
and PhP
there to ensure we also support case insensitive language metastring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, i already tried doing that before making a pr.
But only php code block works and other like you mentioned doesn't highlight code.
I thought our only goal was to allow the user to put a language in additionalLanguages array in any case but he should enter the language in only lower case when specifying the language of code block. Something like this
But, do we also want to add highlight support for code blocks like these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I have added them and I also saw that the VSCode Markdown preview supports highlighting all these blocks. So, maybe we should do it too.
I think this should be done within theme config validation, where we map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, almost good but only 1 of the 3 examples there is highlighted, while I expect the 3 examples to be ;)
@@ -176,7 +176,7 @@ export default function themeClassic( | |||
|
|||
configureWebpack() { | |||
const prismLanguages = additionalLanguages | |||
.map((lang) => `prism-${lang}`) | |||
.map((lang) => `prism-${lang.toLowerCase()}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be better handled in the config validation, you can use Joi to normalize/lowercase the provided user input values so that everything else in the system see lowercase values (including prism-include-languages.ts
, it shouldn't be needed to lowercase there too anymore)
?> | ||
``` | ||
|
||
```PHP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test added, but unfortunately highlighting doesn't work yet
We probably need to lowercase the meta string language in the code block theme component too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should support the other two as well. |
@slorber after two weeks I tried working on this PR again. now, I noticed that my code changes are not being picked up as I tried to redo the changes of PR that I have made in the local setup. For example I am using a bad case in additionalLanguages array for php like After reverting the changes I think it should have broken but it didn't. The changes not being picked up is the reason why I cannot work further on the meta string thing you mentioned (even though I guessed where to make changes) I run |
@slorber It feels a bit odd to me 😅😅😅, I mean you did all the work yet it looks like I have made the changes by creating a PR. |
I just polished a bit what you started so that we can merge 😄 I do that often and it's faster to move on than back and forth PR discussions |
Yes that makes a lot of sense |
Pre-flight checklist
Motivation
fix #9012
If someone uses a language that is not supported by Docusaurus by default then you have to add that language in
additionalLanguages[]
indocusaurus.config.js
. If a language added is not in lowercase, then highlighting for that language will not work. For exampleAdding
'PHP'
or'pHp'
instead of'php'
Test Plan
additionalLanguages: ['php'],
additionalLanguages: ['PHP'],
additionalLanguages: ['pHp'],
Test links
https://deploy-preview-9183--docusaurus-2.netlify.app/tests/pages/code-block-tests/#code-block-prism-language-tests
Related issues/PRs
#9012