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

Enhance code example block: line numbers, highlight lines, line anchors #6372

Closed
wants to merge 4 commits into from

Conversation

OnkarRuikar
Copy link
Contributor

Summary

The PR enhances the code example block by adding a custom plugin and a hook to existing Prism.js syntax highlighter.
It implements the following features:

  • line numbers (configurable in markdown code bock)
  • highlight specified lines (configurable in markdown code bock)
  • provide anchor to the code lines

With highlighted lines it's easy to pinpoint the code being discussed. Readers won't have to read the entire code, thus it'll will save their time and maintain the focus.

The line numbers can be used in the documentation around the code block. Currently, there are discussions on the site which point to line numbers in the corresponding code blocks, making readers count and remember the lines. Also, due to wrapping it could be hard for readers to locate the lines.

Anchor tags can be used on other pages to point to exact line in the code. Also can be used on other websites.


Here is a demo:

lineNo.mov

Screenshots

Chrome Mobile

Safari

Notice the third line in JavaScript code is wrapped. The numbering makes it easy to realize and readers won't wonder if there are two lines. The implementation puts the code comment on one line.

Following is the markdown text used for the CSS code example:

```css linenumbers highlight-2-6
button {
  cursor: pointer;
}

button:hover {
  border: 1px dashed red;
}
```

The info string linenumbers makes the feature opt-in. We wouldn't want some code blocks be numbered, e.g. syntax blocks, command lines, logs etc. This way we can integrate this in mdn/content at our own pace.
The pattern highlight-2-6 says we want to highlight line 2 and 6. If this doesn't look good then we can come up with a better human and code friendly pattern.


How did you test this change?

Tested on the latest Firefox, Chrome, and Safari.

@schalkneethling schalkneethling added the 🧑‍🤝‍🧑 community contributions by our wonderful community label May 27, 2022
@schalkneethling
Copy link

@OnkarRuikar Thank you for your work! 🎉 One clarifying question, does this build on top of #6083 or, is it intended to be a different approach to solving the same issue?

@OnkarRuikar
Copy link
Contributor Author

One clarifying question, does this build on top of #6083 or, is it intended to be a different approach to solving the same issue?

This is a different approach. Honestly these features were in my mind for a long time. After seeing the discussion on #6083, I finally decided to implement it.

Unlike that PR this one doesn't create horizontal scrollbar in code sections. Horizontal scrollbars is a bad UX.
I found these solid points advocating not to have it:

  • Most mice have a vertical scrolling wheel. This means most users have to manually operate the scroll mechanism. It's very easy to use the wheel.
  • We’re so used to reading left to right within the confines of a page where we make our way slowly downwards, introducing a horizontal scroll could break a fairly rigid western convention so should be used with care when reading is a core part of the user journey.

I don't know how much of this is applicable to code blocks. But we are not displaying complete code files like GitHub or IDEs.

@teoli2003
Copy link
Contributor

teoli2003 commented Jun 3, 2022

Historical note: we had these features (line numbers and highlighting) a long time ago (Some articles still mention line numbers.) and we lost them when we migrated to Kuma (yes, Kuma, not Yari). They were very valuable for guide articles for example.

Historical note 2: I think we removed the last few highlight-x-y during the migration to MD as this was long gone.

cc/ @wbamberg and @hamishwillee as I'm curious about their opinions here.

Edit: removed the question that was answered earlier in the thread

@teoli2003
Copy link
Contributor

teoli2003 commented Jun 3, 2022

Also, the highlight-2-6 won't work without linenumbers.

  • Will this raise a prism error in the console, as it does now when we are not using a known language?
  • Also, will it generate an error (in the console) if we have highlight-2-6 when there is no line 6 in the block?

(Yes, I'm really keen to maintain the integrity of our Markdown sources)

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Jun 3, 2022

Thank you for the suggestions! I'll make both the changes.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jun 3, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Two nits, and I or a colleague would have to take a closer, but before we do that, just to verify: Did you see https://prismjs.com/plugins/line-numbers/ by any chance and try out if that (simpler solution) would work for us at all?

build/syntax-highlight.js Outdated Show resolved Hide resolved
client/src/app.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added merge conflicts 🚧 Please rebase onto or merge the latest main. and removed dependencies Pull requests that update a dependency file labels Jun 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jun 3, 2022
@wbamberg
Copy link
Collaborator

wbamberg commented Jun 3, 2022

Although I appreciate the work in this PR, personally I'm -1 on this change. When we migrated to Markdown we discussed highlighting and line numbers, and decided not to take it with us: mdn/content#3512 , basically for the reasons Chris gave in mdn/content#3512 (comment).

I don't think line numbers are very useful in shorter code samples on MDN, and longer code blocks are usually not good documentation anyway (they are better as separate example sites).

Line numbers being optional makes the already complex authoring interface for MDN more complex, and raises questions like, what should our practices be here? When should we use it and when not? What are readers expected to understand when they see this inconsistency?

I think highlighting creates (and in our past created) a maintenance problem, where the code gets updated and the references don't.

I think the main problems of MDN are not a lack of features, but are maintainability, consistency, and correctness of content, which mostly stem from a long time of a very complicated authoring platform which most people didn't really understand how to use. So I'm very much in favour of simplifying things, and think we should be conservative about adding features.

But, I appreciate that this is a very opinionated take, and there are likely to be other opinions.

@OnkarRuikar OnkarRuikar requested a review from caugner June 3, 2022 17:55
@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Jun 5, 2022

Some of the popular documentation sites do use highlighting even for small blocks:
small code with highlighting
Here we can quickly spot what the discussion is all about. We don't have to necessarily spend time on reading the entire code description. Reading that site is a breeze.

Most of the readers land on a documentation page from google search, looking for finding solutions for their problem quickly. They are always in hurry, and they mostly scan the page. Very few read the page from top to bottom like a text book.
With this feature, readers who are in hurry won't necessarily have to read the surrounding text to get the point/solution. Experienced developers or those who have already read the page will find this useful and time saving.

Some of us find the feature useful. Form the mentioned discussion:

I agree with you @chrisdavidmills that highlighting is a pain for the author, but IMO it can be better for the reader, even in quite small blocks of code.

Also, authors will be happy to take the small extra effort to give better user experience. They are always eager to add extra decorations to their content.


longer code blocks are usually not good documentation anyway (they are better as separate example sites).

Agreed. But we can't always force authors to have small code blocks and rearrange their content significantly. Arguing with them on the size of code blocks won't be encouraging. Also, there are exiting long code blocks.

I think highlighting creates (and in our past created) a maintenance problem, where the code gets updated and the references don't.

But we have a strong review system now. As per my experience, the reviewers here are very efficient and they do not let a single mistake pass.

@schalkneethling
Copy link

Hey @OnkarRuikar and @wbamberg any objections if I add your comments to mdn/mdn-community#111 - I would like us to have all of this feedback in one place to discuss and agree on how we want to move forward concerning how code examples are displayed. Ideally, this should be a conversation between content, engineering, and community feedback.

@OnkarRuikar
Copy link
Contributor Author

Hey @OnkarRuikar and @wbamberg any objections if I add your comments to mdn/mdn-community#111.

No objection from my side. We need more views on this.

@hamishwillee
Copy link
Contributor

hamishwillee commented Jun 5, 2022

When it is appropriate, even in short chunks of code, I find it much easier to understand code blocks where the relevant part is highlighted.

So while I accept all the arguments against, on balance I am strongly for this change - even though this does encourage poor writing practices as it is used like a silver bullet. That will be no surprise to anyone, since I proposed this in #2757

@hamishwillee
Copy link
Contributor

PS Once we make a decision this time we should enshrine the decision so we don't go around the loop again (even if that happens to be against my preference)

@schalkneethling
Copy link

Thank you for your input @hamishwillee. I have also linked to it from the discussion here: https://github.com/orgs/mdn/discussions/111

@teoli2003
Copy link
Contributor

teoli2003 commented Jun 7, 2022

We discussed this in the Editorial Meeting yesterday.

We agreed that there are cases where this can lead to an improvement, but we came to the following consensus:

We are working hard for months, and years, to simplify the docs system, using source control, migrating to MarkDown, removing many macros, … This feature will add extra complexity and works against what we are trying to achieve.

So we think that we should not add this, at least for the time being. When we are done with our simplification of the stack/system, we will be open to reconsidering this, but let's not start this for the moment.

We should close this PR for now.

@caugner caugner removed their request for review June 7, 2022 11:27
@schalkneethling
Copy link

Hi there everyone,

We discussed how we display code examples on MDN Web Docs, and for the moment, we are going to move all of this to a discussion for the following reasons:

  1. Different people have different ideas and suggestions for improving the code examples on MDN Web Docs, but we need to reach a consensus before starting to implement any of this. There are many aspects to consider, such as the complexity it might add to the codebase, additional dependencies, and potentially an increased maintenance burden.
  2. We can justify an increase in complexity or even maintenance if we are sure that the change(s) we make has a positive UX impact for all users across devices.
  3. We also need to ensure that changes do not complicate the authoring experience
  4. Keep accessibility and performance in mind.

Keeping all of this in mind, we thought it best to close this pull request and #6083 and continue the discussion between staff and external staff contributors and the wider community to find a solution that benefits everyone.

Thank you for opening this pull request which aided in getting this conversation started. We believe in the long term, this will lead to a better experience for everyone using MDN Web Docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍🤝‍🧑 community contributions by our wonderful community
Projects
Development

Successfully merging this pull request may close these issues.

6 participants