docs: show red underlines in examples in rules docs #18041

ota-meshi · 2024-01-27T07:23:57Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR modifies docs/src/_plugins/md-syntax-highlighter.js to indicate where an error occurs with a red marker in the examples in the rules document.

Is there anything you'd like reviewers to focus on?

close #16227

This process is done using Prism's after-tokenize hook.
~~Content that starts with /* eslint and whose language is js is considered an example of an eslint rule.~~
It is now consider it an example of an ESLint rule only if it is wrapped in a custom container (that is, if parser options are passed from the custom container's rendering process).
I would like to hear your opinion if this will work.
I would like to hear your opinion on what style should be used.

netlify · 2024-01-27T07:24:02Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`f90b1d2`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65e8fff0d610930008c0423c
😎 Deploy Preview	https://deploy-preview-18041--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

ota-meshi · 2024-01-27T07:35:35Z

It seems that CI does not install root dependencies, which causes the documentation to fail to build.
Can you temporarily change to install root dependencies to see how this PR changes the documentation?

ota-meshi · 2024-01-27T07:37:40Z

It looks like this on my local.

fasttime · 2024-01-27T09:50:44Z

Thanks for this PR! CI is failing because the job that builds the docs now requires ESLint dependencies to be installed, but only deps in the docs package (in the subfolder) are installed at that point:

eslint/.github/workflows/docs-ci.yml

Lines 34 to 36 in 1bf2520

    
           - name: Build Docs Website 
        
             working-directory: docs 
        
             run: npm run build

We could run npm install in the parent folder before running the build script, or we could move this particular job to the main workflow in .github/workflows/ci.yml and have it running from there. But in this case, maybe we'll need to run npm install in the docs package as well. You should be able do all changes by yourself, I think.

ota-meshi · 2024-01-27T12:05:45Z

docs/package.json

@@ -20,7 +20,8 @@
        "lint:scss": "stylelint \"**/*.{scss,html}\"",
        "lint:fix:scss": "npm run lint:scss -- --fix",
        "build:minify-images": "imagemin '_site/assets/images' --out-dir='_site/assets/images'",
-        "start": "npm-run-all build:sass build:postcss --parallel *:*:watch"
+        "start": "npm-run-all build:sass build:postcss --parallel *:*:watch",
+        "postinstall": "cd .. && npm install -f"


I temporarily added postinstall so that we can check the results of this PR with netlify. I think we should probably replace this PR with another method before merging.
https://deploy-preview-18041--docs-eslint.netlify.app/rules/array-callback-return

We can add the top-level install to the workflow. 👍

I removed the postinstall and changed the workflow.
I think we probably need to modify the netlify script, but how should I do that?

Hmm. There is no Netlify script. It just checks out docs and does npm install.

I wonder, could we just list "eslint": "file:../" in docs/package.json and that will work?

I wonder, could we just list "eslint": "file:../" in docs/package.json and that will work?

I tried that locally but it doesn't seem to work. eslint dependencies were not installed 😓

Do you think putting the netlify.toml and adding the command config will work fine?
https://docs.netlify.com/configure-builds/file-based-configuration/#build-settings

Let's try it. I'm out of other ideas.

I tried adding netlify.toml and it seems to be working fine 🎉

https://deploy-preview-18041--docs-eslint.netlify.app/rules/array-callback-return

docs/src/_plugins/prism-eslint-installer.js

docs/tools/prism-eslint-hook.js

nzakas · 2024-01-29T18:14:55Z

I think it's okay to update the CI in this PR.

This looks really good, thanks! Can we change the red underline to look more like the one VS Code uses? (A little squiggly.) I'm pretty sure it's just a repeating image that we can find in the VS Code repo.

docs/tools/prism-eslint-hook.js

amareshsm · 2024-01-30T21:06:59Z

docs/tools/prism-eslint-hook.js

+
+/**
+ * Add content that needs to be marked.
+ * @param {string} content Source code content that marks eslint errors.


Suggested change

* @param {string} content Source code content that marks eslint errors.

* @param {string} content Source code content that marks ESlint errors.

nzakas · 2024-01-31T18:44:47Z

This is looking really nice. Is it possible to add a tooltip over the red-underlined code that includes the error message?

ota-meshi · 2024-01-31T23:37:25Z

Is it possible to add a tooltip over the red-underlined code that includes the error message?

Hmm. It seems difficult to do that with Prism's hooks for now.
It doesn't seem possible to add anything other than the <span> class and content for each Token. (For example, we cannot add the popovertarget attribute only to the error token.)
If we request the Prism team to pass the Token instance to Prism's wrap hook, we might be able to get a tooltip, but I'm not sure yet 🤔

ota-meshi · 2024-02-01T10:34:18Z

If we request the Prism team to pass the Token instance to Prism's wrap hook, we might be able to get a tooltip, but I'm not sure yet 🤔

I tried it but it didn't work well. During the after-tokenize hook and before, all Token instances created are replaced with new normalized Token instances before reaching the wrap hook.

nzakas · 2024-02-01T17:59:55Z

Is it possible to add a title attribute to the <span>?

ota-meshi · 2024-02-02T00:05:28Z

I don't know how to add title attribute (an attribute other than class) to a token using Prism's API 😓.
It might be possible to give a unique key to a class and later retrieve the message that maps to the unique key and set it to the title. But it may seem a bit like a hack.

ota-meshi · 2024-02-02T02:10:27Z

It might be possible to give a unique key to a class and later retrieve the message that maps to the unique key and set it to the title.

I tried that. If someone doesn't like it that way, we can change it back.
I also made major changes to prism-eslint-hook.js because we needed to organize the token structure more to display the title properly.

https://deploy-preview-18041--docs-eslint.netlify.app/rules/array-callback-return

nzakas

This looks amazing. ❤️

We can update the docs CI to install the top-level packages and I think this is ready to go.

nzakas · 2024-02-02T18:48:40Z

docs/package.json

@@ -20,7 +20,8 @@
        "lint:scss": "stylelint \"**/*.{scss,html}\"",
        "lint:fix:scss": "npm run lint:scss -- --fix",
        "build:minify-images": "imagemin '_site/assets/images' --out-dir='_site/assets/images'",
-        "start": "npm-run-all build:sass build:postcss --parallel *:*:watch"
+        "start": "npm-run-all build:sass build:postcss --parallel *:*:watch",
+        "postinstall": "cd .. && npm install -f"


We can add the top-level install to the workflow. 👍

fasttime · 2024-02-04T17:23:03Z

Nice work! LGTM when the build is fixed.

mdjermanovic · 2024-02-04T17:46:03Z

docs/tools/prism-eslint-hook.js

+const { Linter } = require("../../lib/api");
+const astUtils = require("../../lib/shared/ast-utils");


How would this work for https://github.com/eslint/zh-hans.docs.eslint.org, where we don't have these modules?

Oh... I don't think it will work as it is now.
@kecrily Do you have any ideas how to solve it?

What do you think about changing this PR as below?

astUtils is used only to get the regex, so it inlines the regex.

Try require("../../lib/api").Linter and require("eslint").Linter and use the one that does not cause an error as the Linter.

Change the install script of zh-hans.docs.eslint.org to install eslint as a dependency.

Another option is to not do red underlines outside of the US site. We could check to see if these utilities exist, and if not, just don't do the linting.

Another option is to not do red underlines outside of the US site. We could check to see if these utilities exist, and if not, just don't do the lining.

I'd prefer this option.

Another option is to not do red underlines outside of the US site. We could check to see if these utilities exist, and if not, just don't do the linting.

I've made changes to this PR to make it so 👍

docs/src/assets/scss/syntax-highlighter.scss

snitin315

This looks great ⭐ , thanks!

Leaving it open for @mdjermanovic

docs/src/assets/scss/syntax-highlighter.scss

mdjermanovic · 2024-03-04T16:39:15Z

docs/tools/prism-eslint-hook.js

+            if (
+                env.content === "" ||
+                env.content === "\n" ||
+                env.content === "\r\n" ||
+                env.content === "\ufeff"
+            ) {
+                env.classes.push(CLASS_ESLINT_MARKED_ON_ZERO_WIDTH);
+            }


In this case, nothing is underlined in the fourth example as the reported text is \n\n:

https://deploy-preview-18041--docs-eslint.netlify.app/rules/padded-blocks#never

VS Code, for example, always underlines linebreaks at the start of the range, and linebreaks of empty lines within the range (so that the range always looks continuous), so this example looks like this:

Would it be possible to do something similar? If not, could we maybe also add CLASS_ESLINT_MARKED_ON_ZERO_WIDTH in case the entire content consists of only linebreaks?

I changed this PR to make it look like VSCode by extracting the newline code character by character into tokens 👍

Would it be possible to do this only for newline characters that appear in an error range and are either at the very start of the range or lone on the line?

I added CSS and changed it to look like that.

Looks great, thanks!

amareshsm · 2024-03-05T18:33:25Z

nice 🎉

mdjermanovic

Great work, thanks!

ota-meshi requested a review from a team as a code owner January 27, 2024 07:23

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Jan 27, 2024

fasttime mentioned this pull request Jan 27, 2024

Change Request: Show red underlines in examples in rules docs #16227

Closed

1 task

fasttime added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jan 27, 2024

ota-meshi commented Jan 27, 2024

View reviewed changes

fasttime reviewed Jan 27, 2024

View reviewed changes

docs/src/_plugins/prism-eslint-installer.js Outdated Show resolved Hide resolved

ota-meshi force-pushed the red-underlines branch from 1c54f4c to 2b6af6b Compare January 28, 2024 01:48

fasttime reviewed Jan 28, 2024

View reviewed changes

docs/tools/prism-eslint-hook.js Outdated Show resolved Hide resolved

ota-meshi mentioned this pull request Jan 29, 2024

docs: show red underlines in examples in rules docs eslint-stylistic/eslint-stylistic#273

Merged

fasttime reviewed Jan 29, 2024

View reviewed changes

docs/tools/prism-eslint-hook.js Outdated Show resolved Hide resolved

docs/tools/prism-eslint-hook.js Outdated Show resolved Hide resolved

docs/tools/prism-eslint-hook.js Outdated Show resolved Hide resolved

amareshsm reviewed Jan 30, 2024

View reviewed changes

nzakas reviewed Feb 2, 2024

View reviewed changes

mdjermanovic reviewed Feb 4, 2024

View reviewed changes

github-actions bot added github actions and removed documentation Relates to ESLint's documentation labels Feb 19, 2024

ota-meshi added 3 commits February 19, 2024 09:43

docs: show red underlines in examples in rules docs

11ead30

fix: add postinstall to install eslint dependencies

bc1c2ac

fix: after-tokenize hook

cb53d9a

fix: for unicode-bom with always

11622c1

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Feb 27, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Feb 27, 2024

fasttime reviewed Feb 28, 2024

View reviewed changes

docs/src/assets/scss/syntax-highlighter.scss Outdated Show resolved Hide resolved

feat: improved styles for zero width markers.

8398ad6

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Feb 28, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Feb 28, 2024

chore: fix format

78d9858

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Feb 28, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Feb 28, 2024

fasttime approved these changes Feb 29, 2024

View reviewed changes

snitin315 approved these changes Mar 3, 2024

View reviewed changes

mdjermanovic reviewed Mar 4, 2024

View reviewed changes

docs/src/assets/scss/syntax-highlighter.scss Outdated Show resolved Hide resolved

Update syntax-highlighter.scss

541dbd1

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Mar 4, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Mar 4, 2024

mdjermanovic reviewed Mar 4, 2024

View reviewed changes

fix: issue with consecutive line breaks

0d2c80d

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Mar 5, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Mar 5, 2024

nzakas approved these changes Mar 5, 2024

View reviewed changes

fix: hides marker on newline following the marker

f90b1d2

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Mar 6, 2024

github-actions bot removed the documentation Relates to ESLint's documentation label Mar 6, 2024

mdjermanovic added the documentation Relates to ESLint's documentation label Mar 7, 2024

mdjermanovic approved these changes Mar 7, 2024

View reviewed changes

mdjermanovic merged commit d961eeb into eslint:main Mar 7, 2024
19 checks passed

ota-meshi deleted the red-underlines branch March 7, 2024 10:37

mdjermanovic added the contributor pool label Mar 7, 2024

	* @param {string} content Source code content that marks eslint errors.
	* @param {string} content Source code content that marks ESlint errors.

		const { Linter } = require("../../lib/api");
		const astUtils = require("../../lib/shared/ast-utils");

docs: show red underlines in examples in rules docs #18041

docs: show red underlines in examples in rules docs #18041

Conversation

ota-meshi commented Jan 27, 2024 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Jan 27, 2024 • edited

✅ Deploy Preview for docs-eslint ready!

ota-meshi commented Jan 27, 2024

ota-meshi commented Jan 27, 2024

fasttime commented Jan 27, 2024

ota-meshi Jan 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Jan 29, 2024

Choose a reason for hiding this comment

nzakas commented Jan 31, 2024

ota-meshi commented Jan 31, 2024

ota-meshi commented Feb 1, 2024

nzakas commented Feb 1, 2024

ota-meshi commented Feb 2, 2024

ota-meshi commented Feb 2, 2024

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitin315 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amareshsm commented Mar 5, 2024

mdjermanovic left a comment

Choose a reason for hiding this comment

ota-meshi commented Jan 27, 2024 •

edited

netlify bot commented Jan 27, 2024 •

edited

ota-meshi Jan 27, 2024 •

edited