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 Expressive Code to Starlight #742

Merged
merged 28 commits into from
Nov 20, 2023

Conversation

hippotastic
Copy link
Contributor

What kind of changes does this PR include?

  • Changes or translations of Starlight docs site content
  • Changes to Starlight code

Description

  • Integrates astro-expressive-code into Starlight
  • Adds an expressiveCode config option & schema that allows configuring the integration.
  • Adds expressiveCodeI18nSchema including all translatable strings, similar to the Pagefind schema.
  • Adds information about the configuration options and i18n to the docs.
  • Closes Copy button for Code Blocks #461

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2023

🦋 Changeset detected

Latest commit: 46103c7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify

This comment was marked as outdated.

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Sep 22, 2023
@HiDeoo
Copy link
Member

HiDeoo commented Sep 22, 2023

This is looking super great 🎉 Amazing work!

I'll play more with it later and make a more thorough review but I have a few small questions:

  • Is the font change expected, shouldn't it respect --sl-font-system-mono? Same question for the font size.
  • I see you are using color-mix() which seems to have landed fairly recently on some browsers, e.g. March 2023 on Chrome, May 2023 on Firefox, and 10 days ago on Chrome for Android if I read that correctly. Could it be a concern? Wouldn't it be a bit too early to use it?
  • "file names/titles" in code block are not selectable, is that expected? I have to say, even more for new files, it's super handy to be able to quickly select it to copy it. At least that would be my expectation.

Again, great work! 👏

@hippotastic
Copy link
Contributor Author

hippotastic commented Sep 22, 2023

Thank you for the valuable feedback, @HiDeoo!

  • Great catch regarding the font family & font size! I have now referenced the CSS variables var(--__sl-font-mono) and var(--sl-text-code) that Starlight also uses for inline code.
  • The file names / titles were not selectable due to a pseudo-element blocking the inputs. I have also fixed this. Thank you for testing so thoroughly!
  • I intentionally only used color-mix() for a single border style that looked too prominent otherwise. If the browser does not support this function, the border will simply become transparent, which does not impact the overall look and usability in my opinion.

@HiDeoo
Copy link
Member

HiDeoo commented Sep 23, 2023

Wow, thanks for the quick and efficient updates and answers! Really appreciate it.

@kevinzunigacuellar
Copy link
Sponsor Member

kevinzunigacuellar commented Sep 24, 2023

Small nits,

  • code blocks were slightly updated from sharp to rounded corners.
  • the syntax highlight seems to be different from the previous default
Before After
image image

They both look very good but I think we might want to keep the defaults as close as possible

@hippotastic
Copy link
Contributor Author

Both correct, good observations!

The slightly rounded corners could be turned off by providing a styleOverrides setting of { borderRadius: '0' }. However, I honestly like the rounded ones better - especially in combination with the tabs. Starlight also has rounded corners in other places like the next/previous page navigation buttons at the end of pages, so I hoped that this is close enough. 😄 If it's a hard requirement to keep the previous style, we could of course change the defaults.

The different syntax highlighting colors are a result of different default themes. Starlight only supported a single theme before and used custom CSS variables to make this one theme work in both dark and light mode, while Expressive Code supports freely definable separate dark and light themes and uses the GitHub dark and light themes as the default. If you have another VS code / Shiki theme in mind that you'd like to use instead of github-dark, please let me know. We could even develop our own themes, which might be interesting from a branding perspective! I just picked these ones for now because they are already bundled with Shiki and seemed to be a popular choice.

@HiDeoo
Copy link
Member

HiDeoo commented Sep 24, 2023

I have a bit more high-level question (and I am also curious ^^).

With these changes, we would be shifting away from the css-variables theme to the good old shiki approach of rendering & shipping every code blocks twice. According to this comment from Fred, there was a bit of a pushback from the Astro community regarding that last approach, which is what actually led to the css-variables theme. I am wondering if with Starlight's emphasis on page weight, this could also be a concern?

Was any thought or experimentation done regarding using shikiji (Anthony Fu ESM-focused rewrite of shiki) which replaces the css-variables theme by a new interesting approach to support light/dark themes? Or maybe the idea would be to wait until this is ported back to shiki itself, if that ever happens, as this seems to be the plan?

@hippotastic
Copy link
Contributor Author

I'm honestly not a big fan of the CSS variables theme as it's making it impossible to ensure an accessible color contrast ratio when adding annotations to code. Expressive Code puts an emphasis on accessibility by always checking the color contrast and tweaking the syntax highlighting colors when necessary. With CSS variables, it cannot know what the actual syntax token color is going to be, so we might end up with bad accessibility. Due to this, I even intentionally disabled the CSS variables theme in Expressive Code.

@hippotastic
Copy link
Contributor Author

Just had a look at Shikiji, and an approach like this definitely looks more promising than the CSS variables theme to me!

However, themes in Expressive Code are more than just syntax highlighting. The themes provide a full spectrum of UI colors as well, and these will be used by plugins like frames to render editor tabs, terminal windows etc., all of which Shikiji doesn't address because it seems to be focused on syntax highlighting only.

I think we can (and I will) explore approaches for more efficient multi-theming for sure, but this would take a considerable amount of time and should not be done in the scope of this PR in my opinion.

@HiDeoo
Copy link
Member

HiDeoo commented Sep 24, 2023

I totally agree with you regarding the css-variables theme approach to use CSS variables to read the colors for the tokens.

The shikiji new approach to use CSS variables to actually store the colors on each token is a very interesting one indeed and I was curious to get your opinion on it (hence the "more high-level question" comment). Thanks for taking the time to detail how theming works in Expressive Code as this is not something I'm in-depth familiar with and also your plans to explore more ideas. 🙌 Very interesting!

PS: Definitely outside the scope of this PR to experiment with this, I agree.

@hippotastic
Copy link
Contributor Author

Sure thing! I love both Orta's and Anthony's work on this and think that a Shikiji-like approach might work even for UI theming in Expressive Code. Finding a way to get efficient multi-theme output is on my roadmap as well. Thank you for all the valuable input and context!

@hippotastic
Copy link
Contributor Author

hippotastic commented Sep 25, 2023

Ok, so I couldn't get this out of my head and experimented with CSS variable-based approaches to multi-theming inspired by Shikiji - and it seems to work!

One of the next Expressive Code releases will always generate a single output per code block, no matter how many themes you define. The config- & theme-based parts of the UI (everything you can see in styleOverrides) will be output into CSS variables, too, making all the rest of the styles static and reusable.

This migration will take me a while though, so I think it's your decision whether the current version is good enough for Starlight to integrate it now and just upgrade the dependency later when the new rendering is available, or if you'd rather want to wait for the more efficient multi-theme rendering. I know how much we are focused on performance and minimal footprint (and I care a lot about that myself), but I also know how many devs are already waiting for Expressive Code features like text markers or the copy button. 😄 Probably not easy to decide - I'm fine with both options!

@HiDeoo
Copy link
Member

HiDeoo commented Sep 25, 2023

Ok, so I couldn't get this out of my head and experimented with CSS variable-based approaches to multi-theming inspired by Shikiji - and it seems to work!

Oh boy, this is all my fault. I'm so sorry. Altho, I find it excellent news to know that the discussion could lead to some improvements in that area and that their approach could potentially work in a way more complex setup like the one from Expressive Code. 🎉

Personally, and it's only my own opinion of course, I think it would be fine to pursue with the proposed changes in this PR and later, when it's ready, upgrade the dependency. This would mean people can benefit from the amazing Expressive Code features as soon as possible. When I mentioned the double rendering, I did that asking in a question if it could be a concern because I have no idea personally so maybe some other people will have a better idea about it.

@hippotastic
Copy link
Contributor Author

hippotastic commented Sep 25, 2023

No worries at all! It's not your fault, you just provided great input that happened to successfully nerd-snipe me! 😄

@HiDeoo
Copy link
Member

HiDeoo commented Sep 28, 2023

A few more high-level comments:

  • I think the code blocks section in the Authoring Content in Markdown guide could be updated to describe some new syntaxes added by Expressive Code? It may also be worth linking to the Expressive Code docs for Markdown/MDX usage for the frames and text markers plugins?
  • There are many places in the documentation where we could use some Expressive Code text markers, but I guess it may be easier to add them in a follow-up PR. I'll try to prepare a list of places where I think we could use them.

@lorenzolewis
Copy link
Contributor

A bit late to the party so feel free to disregard, but when we were doing some work on other UI components the Starlight design system has a pattern of interactive components having rounded corners and non-interactive having square corners. Here's a link to the Figma file in case you wanted to take a peek: https://www.figma.com/file/GrDkcguAR7tSWPFuLVDixq/Theme---Starlight-Docs?type=design&node-id=1%3A2&mode=design&t=ypLDqrL9TDbNrovJ-1

Regardless, I'm super excited to see this and I know myself (as well as some of our readers over at Tauri) are excited for it! 🥳

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Super excited for this. Making some notes although I know @hippotastic is working on some upstream changes that will need to be integrated here before we merge this, so appreciate everyone’s patience while waiting for this amazing PR!

packages/starlight/integrations/expressive-code.ts Outdated Show resolved Hide resolved
packages/starlight/integrations/expressive-code.ts Outdated Show resolved Hide resolved
packages/starlight/integrations/expressive-code.ts Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@hippotastic
Copy link
Contributor Author

hippotastic commented Nov 20, 2023

Btw, I just updated to the latest version of astro-expressive-code to add the automatic frontmatter block cleanup logic we discussed in the context of Astro Docs (if extracting the tab title from a file name comment inside a frontmatter block causes the frontmatter block to become empty, the entire block is removed). It's also affecting one location in the Starlight docs:

Before:

image

After:

image

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Last two comments and then I think I’ll… merge???

packages/starlight/integrations/expressive-code/index.ts Outdated Show resolved Hide resolved
docs/src/content/docs/reference/configuration.mdx Outdated Show resolved Hide resolved
hippotastic and others added 2 commits November 20, 2023 22:29
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Ladies, gentlemen, hippopotamuses…

I HEREBY DECLARE THIS PULL REQUEST READY TO MERGE 📣

Thank you for the hard work @hippotastic and thanks to everyone who reviewed and gave feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy button for Code Blocks
8 participants