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

Add "css-variables" theme, support dark mode #212

Merged
merged 3 commits into from
Aug 31, 2021

Conversation

FredKSchott
Copy link
Contributor

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is perfect.

@orta
Copy link
Contributor

orta commented Aug 26, 2021

OK, this is actually looking like a "good in theory" but doesn't seem to work in practice:

     <pre class="shiki" style="background-color: var(--code-background)"><code><span class="line"></span>
      <span class="line"><span style="color: #000000">import { getHighlighter } from </span><span style="color: #000000">&#39;../index&#39;</span></span>
      <span class="line"></span>
      <span class="line"><span style="color: #000000">test(</span><span style="color: #000000">&#39;The theme with css-variables renders correctly&#39;</span><span style="color: #000000">, async () =&gt; {</span></span>
      <span class="line"><span style="color: #000000">  const highlighter = await getHighlighter({</span></span>
      <span class="line"><span style="color: #000000">    theme: </span><span style="color: #000000">&#39;css-variables&#39;</span></span>
      <span class="line"><span style="color: #000000">  })</span></span>
      <span class="line"><span style="color: #000000">  const out = highlighter.codeToHtml(</span><span style="color: #000000">&quot;console.log(&#39;shiki&#39;);&quot;</span><span style="color: #000000">, </span><span style="color: #000000">&#39;js&#39;</span><span style="color: #000000">)</span></span>
      <span class="line"><span style="color: #000000">  expect(out).toMatchSnapshot()</span></span>
      <span class="line"><span style="color: #000000">})</span></span>
      <span class="line"></span></code></pre>

I'm about to head out for the night, and offhand I'm not sure what clues to offer, but the constant #000000 doesn't seem to be a hardcoded into shiki anywhere - maybe could be misconfigured a JSON theme?

@FredKSchott
Copy link
Contributor Author

I’ll take a look, I definitely wasn’t seeing this with the loadTheme API when I tested within Astro

@FredKSchott
Copy link
Contributor Author

well, I have no idea how I was able to see this working in Astro. I must have not looked closely enough. Looks pretty clear that VSCode only supports hex values via Regex: https://github.com/microsoft/vscode-textmate/blob/792e2b639b7ec4cac27fdf5f777ce092908a1ef3/src/theme.ts#L127-L134

I'm unclear if this check is something that the VSCode team will be willing to remove, in which case this solution might not be viable. I don't have the bandwidth to dig too much deeper, but I can create an issue in that repo to at least kick off the discussion.

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just a bit concern about the potential naming conflicts with --code- prefix (possible to make it configurable?), but generally this looks good to me (Thanks for the detailed docs!). Let's see how @octref thinks about this.

@FredKSchott
Copy link
Contributor Author

Great point, I think “code” could be replaced by “shiki”

@FredKSchott
Copy link
Contributor Author

@orta alright, I got a new implementation working for this PR, now assuming the VSCode bug does not get fixed. If we think it might get fixed, we can revert back to the original theme.

The new implementation is essentially a more controlled take on @pveyes's original approach to hex->variable mapping. The new theme uses valid hex color code as unique identifiers (#000001, #000002, #000003, etc) to get around the VSCode limitation. Once the theme has been loaded and we have the color map (_colorMap = ['#000001', '#000002', ...) we can replace those with the matching CSS variable that we care about.

Let me know if this is acceptable. I don't love doing the hex->var mapping, but since we have direct access to _colorMap after the theme is parsed the "fix" step is actually pretty lightweight.

If this method isn't acceptable, then I think we're blocked by the limitation linked to above, and I'll probably just go ahead with this solution in user-land (already implemented in Astro in withastro/astro#1208)

@antfu
Copy link
Member

antfu commented Aug 30, 2021

That's an interesting workaround! Nice idea! I am good with it 👍

@orta
Copy link
Contributor

orta commented Aug 30, 2021

I agree, given that it's all happening internally, I don't think it's too hacky of an answer

@FredKSchott
Copy link
Contributor Author

Great, glad this makes sense. In that case, the PR is ready to merge on my end!

@orta orta merged commit 6bf1b3a into shikijs:main Aug 31, 2021
@orta
Copy link
Contributor

orta commented Aug 31, 2021

Should be shipped as 0.9.9

@octref
Copy link
Collaborator

octref commented Sep 16, 2021

Thanks for your contribution @FredKSchott! Please take a look at #33 (comment) though.

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.

4 participants