-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Replace class=language-* w data-language=* to avoid potential Prism bug #1488
Conversation
Deploy preview failed. Built with commit 05e361d https://app.netlify.com/sites/using-styled-components/deploys/59679392a700c41f4d63d6f0 |
Hello fake @KyleAMathews bot 😁 It seems your deploy previews have been failing a lot lately, based on other recent PRs I see, so I'm going to ignore this message for now. |
Deploy preview ready! Built with commit 05e361d |
Deploy preview ready! Built with commit 05e361d |
Deploy preview ready! Built with commit 05e361d |
Bot Kyle has 1/100 my intelligence and all of my charm :-) |
So I like the solution but how about making this an option with the default false? With this change, prism themes would stop working unless you modify their selector which is doable but more work and confusing. I doubt most people will load prismjs into their page so won't run into the bug you're seeing. |
Sounds great. That was what I had in mind when I mentioned the options flag. Just wanted to run the general approach by you first before spending too much time on it. |
👍 |
Default behavior remains as before, to use a class='language-*' prefix. New behavior allows for a data-language=* alternate. Snapshot tests added.
What do you think of this approach? |
Or check out the alternate approach, #1491 |
Closing in favor of #1491 |
Potential bug fix for issue #1486
Note that this would be a breaking change in terms of custom themes. If this approach seems reasonable going forward, I could hide it behind an option flag.