OpenZeppelin / docs.openzeppelin.com

Source for the OpenZeppelin documentation site
https://docs.openzeppelin.com
45 stars 87 forks source link

Install step copy feature on documentation is possibly incorrect #296

Closed TatisLois closed 3 years ago

TatisLois commented 3 years ago

Documentation link: https://docs.openzeppelin.com/contracts/4.x/#install

When I press the copy icon this is saved to my machine $ npm install @openzeppelin/contracts. This throws an error on the terminal of Expected a variable name after this $.

Because the copy command includes the $ symbol. Is that expected or would it be better to only copy the executable part of the command npm install @openzeppelin/contracts?

I'm willing to make this update if it is a bug.

frangio commented 3 years ago

Thanks for the issue. Please open a PR.

The fix should go in 07-copy-code.js.

If we detect that the code being copied is shell commands, we should remove any leading $. We should try to only do this if the code was configured as a shell/console block by inspecting the class of the code element.

TatisLois commented 3 years ago

Thanks for the insight @frangio. I have working code on my local but was wondering if I can get your opinion on an aspect of the solution.

First, I noticed that the wrapping code has a class of language-sh and a data-lang="sh". In my solution, I'm referencing the data attribute and not the class as it's generally more reliable and prefered over classes. Does that sound acceptable in your opinion?

Second, I noticed that for shell commands we are using language-sh and a data-lang="sh" but in the highlight.js docs they support multiple shell related syntax and aliases. source: https://github.com/highlightjs/highlight.js/blob/main/SUPPORTED_LANGUAGES.md

Should I account for all possible shell languages and aliases (future proof) or just the currently used language-sh/ data-lang="sh"?

Thanks again!

frangio commented 3 years ago

Using data-lang is fine.

Let's try to support all aliases, because we're not being consistent across the docs and we use different kinds of language names. The following should suffice: console, shell, sh, bash.

TatisLois commented 3 years ago

Thanks @frangio, I opened the pull request.

TatisLois commented 3 years ago

Thanks for the review @frangio I updated the PR https://github.com/OpenZeppelin/docs.openzeppelin.com/pull/297 and opened a second one for the babelXRollup https://github.com/OpenZeppelin/docs.openzeppelin.com/pull/298