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

feat: add addThemeNameToClass option, fixes #373 #376

Closed
wants to merge 2 commits into from

Conversation

sesgoe
Copy link

@sesgoe sesgoe commented Nov 26, 2022

  • Add a test if possible

  • Format all commit messages with Conventional Commits
    Did my best here, unsure if this is proper formatting, happy to fix with a squash commit if necessary.

Added a new addThemeNameToClass option for HtmlOptions to enable adding the theme name to the generated HTML class. This leaves the current functionality as-is and makes this new feature opt-in for anyone that wants it.

Previous behavior (and with the option disabled by default):

<pre class="shiki">...</pre>

New Behavior (with the option enabled):

<pre class="shiki nord">...</pre>

I'm unsure about testing conventions here, so I added a test to both simple.test.ts and comprehensive.test.ts that should exercise the functionality. All tests continue to pass with these changes.

There is still a failing test in styleAttributes.test.ts but it's also failing inside the main branch. I don't have the full context to feel comfortable changing it to pass, but I would be happy to add changes necessary to fix it if it's simple and doesn't muddy the understanding of this PR.

@netlify
Copy link

netlify bot commented Nov 26, 2022

Deploy Preview for shiki-matsu failed.

Name Link
🔨 Latest commit 16b2006
🔍 Latest deploy log https://app.netlify.com/sites/shiki-matsu/deploys/63826d4ea5af540009631969

@muenzpraeger
Copy link
Collaborator

I believe it would be more flexible to expose the elements option in codeToHtml - user based overrides are actually already implemented there.

return renderToHtml(tokens, {
   fg: _theme.fg,
   bg: _theme.bg,
   lineOptions: options?.lineOptions,
   elements: options?.elements. // Adding this
 })

Elements are actually a way to have a user-override over pre/code/etc, as one can see here.

And then you can just create your custom <pre> block as you see fit.

const out = highlighter.codeToHtml(`console.log('shiki');`, {
 lang: 'js',
 elements: {
   pre({ className, style, children }) {
     return `<pre class="${className} from muenzpraeger" style="${style}">${children}</pre>`
   }
 }
})

@sesgoe
Copy link
Author

sesgoe commented Nov 29, 2022

I agree with you that this would be more flexible. I feel like the API is much less intuitive/simple, but I can work on implementing this version instead and see how it looks!

Thanks for the feedback.

@muenzpraeger muenzpraeger mentioned this pull request Dec 6, 2022
3 tasks
orta added a commit that referenced this pull request Dec 18, 2022
@orta
Copy link
Contributor

orta commented Dec 18, 2022

Hey folks, good PR!

I think we don't need an option for this, it's just a generally good idea IMO, so I've taken this PR and reduced it down to always add the theme name with 961cd9e

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

Successfully merging this pull request may close these issues.

Dark mode support mentioned in docs does not work
3 participants