-
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
Highlight empty lines without breaking line numbering or hiding them #18719
Conversation
Fixes: #16994
Remove comments that made prettier angry
@@ -154,7 +154,9 @@ module.exports = function highlightLineRange(code, highlights = []) { | |||
}) | |||
.map(line => { | |||
if (line.highlight) { | |||
line.code = highlightWrap(line.code) | |||
line.code |
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.
better check for empty string or null? Otherwise a line with number 0
and text false
would be also exchanged?
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 checked what you suggested and lines where only word to highlight is "false" (without quotes or 0 are highlighted with no issues.
PS. @muescha is there a way to get rid of prettier complains in build errors? I am editing this in the browser.
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.
Snapshot tests are failing as I am injecting non breakable space into empty lines ( ) so these tests should just get the snapshots updated.
Can you move the if check inside https://github.com/gatsbyjs/gatsby/pull/18719/files#diff-5fd2ab3304a334d4977bdcb27e492e3eR54 |
Hi @ethernal 👋 Do you have time to finish this PR? |
@LekoArts I hope to finish it this weekend. I would be grateful for guidance as my experience with GitHub is limited. I know I need to fix the tests as they fail due to introduction of Is there a way I can test all of it locally? |
@ethernal Hi, do you have time to wrap up this PR? You can read up on setting up the repository here: https://www.gatsbyjs.org/contributing/setting-up-your-local-dev-environment/ |
Looks like this was fixed in #21711 and as such and seeing how there's no updates in this PR in a while I'm going to close this PR. If this is incorrect please re-open and update it. Thanks :) |
I'm not sure this was fixed in #21711. I'm still experiencing the issue in #17849 & manually editing I think my problem is because #21711 helps style the empty lines better, but it seem to actually insert the empty line like this PR's edit does, but I'm not sure |
- Didn't fix issue with line highlighting though. - Looks to be related to gatsbyjs/gatsby/issues/17849 - Found fix in PR gatsbyjs/gatsby/pull/18719, implementing manually in `node_modules` helps, but doesn't completely resolve the issue - Committing updates & pushing to v1.4 minor update, but leaving this issue open pending a real resolution.
Highlight empty lines without breaking line numbering or hiding them. See #17849
Description
Allows to highlight empty lines without breaking line numbering and prevents highlighted line from being hidden. Currently that is broken if empty line is highlighted. See #17849 for screenshots and description.
Related Issues
Fixes: #17849