-
Notifications
You must be signed in to change notification settings - Fork 142
Update fenced code block color legibility #2816
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
base: master
Are you sure you want to change the base?
Conversation
Highlight styles are too dark to be legible, and they don't account for dark/light code block themes. Let's * Use CSS variables for highlighter styles * Update site page processing to set data-code-theme on body based on site config * Add in CSS rules that set a suitable highlight colour in the variables based on the data-code-theme
Due to new data-code-theme attribute on body, html output is different. This requires us to update the test script built output to match the expected value.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2816 +/- ##
=======================================
Coverage 71.85% 71.85%
=======================================
Files 134 134
Lines 7340 7340
Branches 1563 1625 +62
=======================================
Hits 5274 5274
+ Misses 2020 1938 -82
- Partials 46 128 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Change made for easy viewing:
is the light grey supposed to be white instead? @Harjun751 |
|
I will update the wording, thanks for flagging that. |
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.
Pull request overview
This PR updates the fenced code block highlighting colors to improve legibility across MarkBind's built-in code themes. The implementation changes the color shorthand system from hardcoded CSS color names to CSS variables that adapt based on the selected code theme.
Changes:
- Modified color mappings in HighlightRule.ts to use CSS variables (e.g.,
var(--red)) instead of hardcoded color names - Added theme-specific CSS variable definitions for light and dark code themes in markbind.css
- Injected
data-code-themeattribute into the body tag via the Nunjucks template to enable theme-aware styling - Updated documentation to show color swatches and descriptions for both light and dark themes
- Updated functional test expected outputs to reflect the new body tag attribute
Reviewed changes
Copilot reviewed 115 out of 116 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/lib/markdown-it/highlight/HighlightRule.ts | Changed color mappings from hardcoded strings to CSS variable references |
| packages/core-web/src/styles/markbind.css | Added CSS variable definitions for 8 colors in both light and dark theme variants |
| packages/core/src/Page/page.njk | Added data-code-theme attribute to body tag for theme-aware CSS selection |
| packages/core/src/Page/index.ts | Passed codeTheme from siteConfig to template context |
| packages/core/test/unit/lib/markdown-it/highlight/HighlightRule.test.ts | Updated test expectations to reflect CSS variable usage |
| packages/core/test/unit/utils/data.ts | Updated mock template with trailing space (incomplete update) |
| docs/userGuide/syntax/code.md | Enhanced documentation with visual color swatches and theme-specific descriptions |
| packages/cli/test/functional/.../expected/*.html (70+ files) | Updated expected HTML outputs with data-code-theme="light" or "dark" attributes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% endif %} | ||
| </head> | ||
| <body> | ||
| <body > |
Copilot
AI
Feb 2, 2026
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.
The PAGE_NJK_LITERAL mock template should be updated to match the actual page.njk template structure. The trailing space after 'body' appears incomplete and doesn't include the new data-code-theme attribute that was added to the actual template in page.njk. While this mock is simplified, it should still reflect the key structural changes for consistency in testing.
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.
I was thinking of this, but figured not to because the other attributes that are set (like data-bs-spy) are not included in this file. TBH, I'm not too sure why. Appreciate direction on this.
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.
It seems like that content of the mock template is only used as a placeholder, (i.e. used to put a file at that path in the fake memory drive for testing) where the actual content inside the file is never read or rendered by any test.
The more important part of the code seems to be
export const PAGE_NJK = {
[path.join(__dirname, '../../../src/Page/page.njk')]: PAGE_NJK_LITERAL,
};
In this case, maybe it would be a good idea to update the PAGE_NJK_LITERAL to
const PAGE_NJK_STUB = '<html><!-- Mock page.njk content --></html>';
so that it doesn't cause confusion like in this case, maybe can open Issue to refactor the testcases, test structure, test stubs etc. and open a PR if you wish to tackle this refactor
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.
LGTM, maybe open an issue regarding PAGE_NJK_LITERAL as per comments? @Harjun751
Not related to the content of the PR though.
| {% endif %} | ||
| </head> | ||
| <body> | ||
| <body > |
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.
It seems like that content of the mock template is only used as a placeholder, (i.e. used to put a file at that path in the fake memory drive for testing) where the actual content inside the file is never read or rendered by any test.
The more important part of the code seems to be
export const PAGE_NJK = {
[path.join(__dirname, '../../../src/Page/page.njk')]: PAGE_NJK_LITERAL,
};
In this case, maybe it would be a good idea to update the PAGE_NJK_LITERAL to
const PAGE_NJK_STUB = '<html><!-- Mock page.njk content --></html>';
so that it doesn't cause confusion like in this case, maybe can open Issue to refactor the testcases, test structure, test stubs etc. and open a PR if you wish to tackle this refactor


What is the purpose of this pull request?
Resolves #2800
Overview of changes:
Updates current code highlighting shorthands to use predefined CSS variables. The variables are set to colors that maintain legibility in two of the respective code themes in markbind. How specific colors are selected is by using an attribute-selector that looks for the data-code-theme value that is set in the body.
The data-code-theme is set by using the site.json values and injecting into the nunjucks template, much like the various other settings.
Anything you'd like to highlight/discuss:
The large amount of files changed is due to the body tag having the data-code-theme attribute set, meaning that many of the functional tests' "expected" files had to change.
You can see how the colours look in the linked issue above.
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Update highlighter styles for legibility
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):