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

docs: add tabs to cli code blocks #18784

Open
wants to merge 94 commits into
base: main
Choose a base branch
from

Conversation

Jay-Karia
Copy link
Contributor

@Jay-Karia Jay-Karia commented Aug 18, 2024

Prerequisites checklist

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

  • 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:

Closes #18783

What changes did you make? (Give an overview)

Adds tabs to cli code blocks for multiple package mangers (yarn, pnpm, and bun).
There's a command: npx eslint [options] [file|dir|glob]* in npm, the code tabs allows users to get the equivalent command in yarn, pnpm, and bun.

@eslint-github-bot
Copy link

Hi @Jay-Karia!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

Copy link

netlify bot commented Aug 18, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 2677b07
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67026dc34213c4000813c637
😎 Deploy Preview https://deploy-preview-18784--docs-eslint.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 configuration.

@Jay-Karia Jay-Karia changed the title feat(docs): add tabs to cli code blocks docs: add tabs to cli code blocks Aug 18, 2024
@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Aug 18, 2024
@Jay-Karia
Copy link
Contributor Author

Jay-Karia commented Aug 18, 2024

While parsing the code_tabs the commands containing ' or " are converted into: ' and " respectively.
npx eslint --rule 'quotes: [error, double]' -> npx eslint --rule 'quotes: [error, double]'

Tried using \', but didn't got it.
Tried npm: `` instead of npm: "", but there remains extra `` in the command

Characters like < and > are also parsed incorrectly

What should we do here.

The pages are Tests, PRs, Custom Parsers, Integration Tutorials, flags, config files and debug.
@harish-sethuraman
Copy link
Member

You should mark the content as safe to disable encoding. {{ params.npm | safe}}

@amareshsm amareshsm added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 18, 2024
@Jay-Karia
Copy link
Contributor Author

The linting errors are due to the commands being wrapped within "".
Such as npm: "npm i eslint"

@Jay-Karia Jay-Karia marked this pull request as ready for review August 19, 2024 15:03
@Jay-Karia Jay-Karia requested a review from a team as a code owner August 19, 2024 15:03
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this. This isn't quite what I had in mind. What I think we need is a separate component that deals with just commands. Something like this:

{{{ install_tabs({
    global: true,
    package: "eslint"
}) }}

{{{ npx_tabs({
    package: "eslint",
    args: ["--fix"]
}) }}

And then the tabs are automatically generated for npm, Yarn, pnpm, and Bun.

We should not be making any changes where we tell people to run npm test, because that's what we use in our repos. Other package managers might not work and there's no need to use them.

@Jay-Karia
Copy link
Contributor Author

@nzakas
Can you please elaborate on components that you mentioned.
And what will be the plan of action ?

@harish-sethuraman
Copy link
Member

There would be two types of tabs for code blocks one being installation and another being the runner.

The installation one takes multiple params like if it needs to be global installation, what package to install, if it is a dev dep etc..

so basically this code:

{{{ install_tabs({
    global: true,
    package: "eslint"
}) }}

will be resolved to

npm i eslint -g
yarn add eslint -g
etc..

similarly for runners we will have the command that has to be run and the args that command takes. So we should auto generate it for all type of runners npx, bunx etc..

@Jay-Karia
Copy link
Contributor Author

There's a library npm-to-yarn which can convert commands.

Is there any other solutions you have thought about ?

@nzakas
Copy link
Member

nzakas commented Aug 20, 2024

These are also very simple mappings from one package manager to another. We don't need any other packages to do the translation for us.

@Jay-Karia
Copy link
Contributor Author

Jay-Karia commented Aug 24, 2024

Should we add <script> inside macro components for generating equivalent commands (for install_tabs) ?

@Jay-Karia
Copy link
Contributor Author

@nzakas Added npx_tabs component, should we start using it in the docs ?

@Jay-Karia Jay-Karia marked this pull request as draft August 24, 2024 17:12
Copy link

github-actions bot commented Sep 3, 2024

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Sep 3, 2024
@Tanujkanti4441
Copy link
Contributor

Tanujkanti4441 commented Sep 4, 2024

@Jay-Karia, CI is failing can you check that?

Tanujkanti4441
Tanujkanti4441 previously approved these changes Sep 26, 2024
Copy link
Contributor

@Tanujkanti4441 Tanujkanti4441 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Would like another team member to review the PR.

@@ -0,0 1,63 @@
{# to npm #}
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this data into a JSON file? I think that would be easier to understand and maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a bit research I found that we need to install extra dependencies for reading the JSON data into the template, and however the majority of the format will be similar to the current one.

If you have another solution to read the JSON data, we can implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

template can read the JSON data, you can check out the src/_data folder in repo.

Copy link
Contributor Author

@Jay-Karia Jay-Karia Sep 28, 2024

Choose a reason for hiding this comment

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

added the data as JSON file and a helper function / file to access the data.
Can you help me with the implementation with the template. As I am new to this :-|

Copy link
Contributor

Choose a reason for hiding this comment

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

seems i am a bit late, thanks for your patience.

What i found is templates with macro.html extension are not reading JSON data directly as other html templates, looking for some other ways to do this, and
would like to ask @eslint/website-team for some suggestions on this.

@Jay-Karia
Copy link
Contributor Author

Jay-Karia commented Sep 27, 2024 via email

@Tanujkanti4441
Copy link
Contributor

Sorry! closed it accidentally.

@nzakas
Copy link
Member

nzakas commented Oct 15, 2024

@Jay-Karia it looks like there's a merge conflict and a lint error, can you take a look.

@Tanujkanti4441 where are we on this?

@Jay-Karia
Copy link
Contributor Author

@Jay-Karia it looks like there's a merge conflict and a lint error, can you take a look.

@Tanujkanti4441 where are we on this?

Will fix them soon after deciding about the JSON file data and implementing.

@Tanujkanti4441
Copy link
Contributor

@Tanujkanti4441 where are we on this?

Looking for a way to implement JSON data in npm_tabs.macro.html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool documentation Relates to ESLint's documentation
Projects
Status: Evaluating
Development

Successfully merging this pull request may close these issues.

Docs: Add tabs to cli blocks
6 participants