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

feat(theme-classic): code block showLineNumbers #7007

Merged
merged 9 commits into from
Apr 13, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Mar 25, 2022

Motivation

It should be possible to show line numbering in code blocks, although this is not a commonly used use case. I once saw a request for such a feature by one our user in some time ago, so believe we can provide it out of the box.
For example Gatsby's code block highlighting plugin has such option, and MkDocs Material originally has such feature.

Therefore code line numbering should not be superfluous, although its implementation is not easy as could be, since it is important to be able to control the color (via opacity) of any line number, which it makes especially more sense when the code lines are highlighted.

image

I specially used in the docs an example with a long code line to demonstrate that the position of a element with line numbers must be fixed with the proper background -- it's important thing.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See docs for example.

https://deploy-preview-7007--docusaurus-2.netlify.app/docs/markdown-features/code-blocks/#code-line-numbering

Related PRs

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 25, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 25, 2022
@netlify
Copy link

netlify bot commented Mar 25, 2022

[V2]

Name Link
🔨 Latest commit 5951f28
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/624ff7429f62250009dfb235
😎 Deploy Preview https://deploy-preview-7007--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Mar 25, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 63
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-7007--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Mar 25, 2022

Size Change: +632 B (0%)

Total Size: 806 kB

Filename Size Change
website/build/assets/css/styles.********.css 106 kB +535 B (+1%)
website/build/assets/js/main.********.js 612 kB +97 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/index.html 38.6 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator

I do know that CSS-only solutions exist, though, like this:

.prism-code {
  counter-reset: line-number;
}

.prism-code .token-line::before {
  counter-increment: line-number;
  content: counter(line-number);
  margin-right: calc(var(--ifm-pre-padding) * 1.5);
  text-align: right;
  min-width: 1.5rem;
  display: inline-block;
  opacity: .3;
  position: sticky;
  left: var(--ifm-pre-padding);
}

Effect: https://www.hscmathematics.com/latex/figures/functions/

I also have another open PR about adding collapsible lines: #5783 But I think Sebastien likes to make people swizzle / use CSS

@lex111
Copy link
Contributor Author

lex111 commented Mar 26, 2022

The CSS-only solution is unfortunately not flexible, in which case there is no way to apply opacity only to text of line number, so you get something like this:

image

Also, a number color of highlighted line should be different from the default, which can be achieved again by introducing an additional empty element. At least I don't see any other easy way to do this.

I understand that it enable line numbering in code blocks is not often necessary, but it does not require much code to implement. As I showed, the CSS-only solution has a disadvantage with long lines, and just creates an inconvenience if line numbering is needed for a certain code block in a document.

@Josh-Cena
Copy link
Collaborator

I see. Yeah, I thought about the same.

@Josh-Cena Josh-Cena changed the title feat(theme-classic): allow line numbering to code blocks feat(theme-classic): code block showLineNumbers Mar 26, 2022
@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Mar 30, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

That looks nice and also something I wanted to see added 👍

Agree that CSS is not good enough in this case and having something available by default seems reasonable

I personally like this design (line highlighting, line hovering etc...): https://blog.maximeheckel.com/posts/advanced-animation-patterns-with-framer-motion/

image

Wonder if it works for live codeblocks? We should probably be consistent and make this available in both live/static code blocks?

@@ -71,6 +71,10 @@ export function parseCodeBlockTitle(metastring?: string): string {
return metastring?.match(codeBlockTitleRegex)?.groups!.title ?? '';
}

export function containsLineNumbers(metastring?: string): boolean {
return metastring?.includes('showLineNumbers') || false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

was thinking maybe we should make it possible to have it enabled by default, and disable on a case-by-case basis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see the need for this, in most cases it is not necessary.

Copy link
Collaborator

@Josh-Cena Josh-Cena Apr 7, 2022

Choose a reason for hiding this comment

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

Yes, I think opt-in is better than opt-out in most cases. It usually adds unnecessary noise, because unless you want to reference one line of code in the main text, you don't need numbering. Another option is to add a default-false theme config option that adds line numbers to all code blocks, but IMO we don't have a strong use-case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option is to add a default-false theme config option that adds line numbers to all code blocks, but IMO we don't have a strong use-case

That's what I was thinking about, but we can add later if requested 👍

````md
```jsx showLineNumbers
function PageLayout(props) {
return <Layout title="Awesome Docusaurus page" description="Just one more awesome page on my Docusaurus site">;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this example is not ideal, maybe add line breaks + find a way to use line highlighting + line numbers in doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deliberately made this example with long line of code to show how line numbering will be positioned when scrolling. About adding line highlighting alongside showLineNumbers, it's good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see thanks

What about making the doc example simpler (not scrolling) and adding all edge cases to a dogfood doc to make review easier? (+10 lines, highlighting, scrolling...)

@lex111
Copy link
Contributor Author

lex111 commented Apr 6, 2022

I personally like this design (line highlighting, line hovering etc...)

I decided to use a design like in editors like VS Code or JetBrains IDE. It seems reasonable to me, considering that the Prism color scheme can change. And it's just more familiar and not as eye-catching (I also mean that the line numbers should be less contrasted by color in comparison with color of code)

Wonder if it works for live codeblocks?

I'm not sure about this, in any case it is a completely different third-party component, perhaps somehow we can add line numbering there, but it is better to do it in the another PR.

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

image

@lex111
Copy link
Contributor Author

lex111 commented Apr 7, 2022

Well spotted, as usual, initially I wanted to do with a minimum amount of code, but in the end (as always) it doesn't work like that, so the final implementation is not so simple, but it covers all possible the use cases (see https://deploy-preview-7007--docusaurus-2.netlify.app/tests/pages/code-block-tests#code-blocks-with-line-numbering-tests)

@lex111 lex111 force-pushed the lex111/code-block-line-numbers branch from e00a86c to bd5a394 Compare April 7, 2022 19:18
@Josh-Cena
Copy link
Collaborator

The right padding causes a weird selection highlight:

image

Probably worth fixing

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

great 👍 LGTM thanks

@slorber slorber merged commit ee4c984 into main Apr 13, 2022
@slorber slorber deleted the lex111/code-block-line-numbers branch April 13, 2022 12:42
@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants