-
Notifications
You must be signed in to change notification settings - Fork 134
Add color for highlighting fenced code blocks #2649
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
Conversation
# Conflicts: # packages/core/src/lib/markdown-it/highlight/Highlighter.ts # packages/core/test/unit/lib/markdown-it/highlight/Highlighter.test.ts
import * as fs from 'fs'; | ||
import * as path from 'path'; | ||
|
||
const siteJsonPath = path.join(__dirname, '../../../../../../docs/site.json'); |
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'm not sure if I understand this correctly. Would this always read the site.json from the docs? It won't read the site.json from the custom site, right?
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.
Yep, it just reads the site.json from the docs, which is wrong. I'm not sure how to extract the codeTheme field from the site.json from the custom site though.
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 that the Site class stores the rootPath
of the user's MarkBind site. Perhaps there's a way to pass that information to this file to get an absolute path to the site.json
.
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.
Thanks for the inputs. I reused the initial logic to handle for the case where I want the default coloring. Initially, highlights are dealt with by the highlighted class. So what I did is to have a conditional check at each highlight, if there is no color specified, I reused the logic of the highlighted class, else, I simply added a custom style depending on the color input.
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.
Thank you for your PR! Have some changes and comments that I think need to be addressed, do let me know if I have misunderstood the logic.
packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/markdown-it/highlight/HighlightRuleComponent.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2649 +/- ##
==========================================
+ Coverage 51.90% 52.03% +0.13%
==========================================
Files 130 130
Lines 7104 7162 +58
Branches 1477 1593 +116
==========================================
+ Hits 3687 3727 +40
+ Misses 3263 3131 -132
- Partials 154 304 +150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Suggested some possible improvements. LGTM!
As for using "@" to indicate color, I am thinking would it be possible to just use "#" will a more robust parsing mechanism? |
Users can specify a color using hex, for e.g. |
I think there was a typo. What I meant was something like |
Currently what the code does is it will split by If split by What about something that is not already present? |
HighlightRule::handleRuleComponent
handleRuleComponent::HighlightRule
On the contrary, given that the convention is to have I agree that the usage of the color highlighting makes the string quite complex though. The example given:
This is quite unreadable to me at this point, and if the person wants more organized color highlighting for their code (e.g. highlight whenever certain keywords appear), this will be quite annoying to modify one by one. I think it's certainly going to be worth discussing how to improve this feature. But as a first stage implementation, I think this is a good first step. |
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.
Thanks for the PR @IanCheah! A few comments mostly on a potential refactor PR after this one (and likely after v6).
Another idea is expanding the colored highlight idea for specific keywords; it might be nice if the user can e.g. pass in a key-value mapping for keywords to colors, and we apply that mapping over a given code block. Maybe we can even expand this to beyond code blocks, to the page (or pages) in general. I think this can be further discussed in a separate issue!
return appliedRule.isUnboundedSlice() | ||
? { highlightType: HIGHLIGHT_TYPES.WholeLine, bounds: null } | ||
: { highlightType: HIGHLIGHT_TYPES.PartialText, bounds: appliedRule.bounds }; | ||
handleLineRange(lineNumber: number): { |
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.
There seems to be quite a few conditional checks in this function... I think that a refactor of these into separate functions might be good for readability and to simplify the logic. There seems to be this:
results.push({
highlightType: HIGHLIGHT_TYPES.<Somthing>,
bounds: null,
color: this.color,
});
That seems to be repeated quite a bit - which I think would also benefit from a refactoring round.
color?: string; | ||
}[] = []; | ||
// Iterate over all rule components to find matches for the current line | ||
this.ruleComponents.forEach((ruleComponent) => { |
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.
Same as above. One too many nested conditionals for my comfort... We can take a look at a separate refactor PR after the v6 release. Seems like we should at least do a refactor round before the highlight logic becomes a bit too convoluted.
What is the purpose of this pull request?
Fixes #2594
Overview of changes:
The new highlight feature
Now users are able to specify the color at each section they want to highlight by adding a
@
followed by a color.Previously, users will specify an inline code highlight as shown below. Depending on the code theme, the highlight will be default to
#e6e6fa
for the light theme and#000000
for the dark theme.java {.line-numbers highlight-lines="1[:],3['Inventory'],3[4::6],4['It\'s designed'],5,6[8:15],6[18:],8[0::2],12[:]-14,16-18,20[12:]-22,24[1::]-26"}
With the new update, users can specify a color they want at each section. For example,
java {.line-numbers highlight-lines="1[:]@pink,3['Inventory']@blue,3[4::6]@orange,4['It\'s designed']@#000000,5,6[8:15]@purple,6[18:]@yellow,8[0::2],12[:]-14,16-18,20[12:]-22,24[1::]-26"}
Those sections of code with no color specified will fallback on the default highlight color in the previous implementation.
Running the above code will result in a inlinecode block that looks like this:

Changes to the documentation
I have added a relevant section under the User Guide. Here is the link to it https://inline-highlight.netlify.app/userguide/formattingcontents#code.
Anything you'd like to highlight/discuss:
Possible Improvements
Currently, users will be able to insert color in the format of either strings like
blue
,red
, etc or hexadecimal representations of colors like#000000
,#e6e6fa
, etc. I intended for formats likergb(230,230,230)
orrgba(255, 87, 51, 0.5)
to be recognized as well, which would be beneficial for people who are proficient with CSS.The use of @
I used
@
to separate the code lines as well as the color. The reason why I chose@
is because I wanted to use thestring::split
method and I had to choose a character that was not already present. Feel free to let me know if there are better/more intuitive way rather than@
Testing instructions:
@color
field behind every comma-separated highlight section.Proposed commit message: (wrap lines at 72 characters)
Add flexibility to highlighting fenced code blocks
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):