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

Install step copy feature on documentation is possibly incorrect #296

Closed
TatisLois opened this issue Aug 19, 2021 · 5 comments · Fixed by #297
Closed

Install step copy feature on documentation is possibly incorrect #296

TatisLois opened this issue Aug 19, 2021 · 5 comments · Fixed by #297

Comments

@TatisLois
Copy link
Contributor

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
Copy link
Contributor

frangio commented Aug 19, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

frangio commented Aug 20, 2021

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
Copy link
Contributor Author

Thanks @frangio, I opened the pull request.

TatisLois added a commit to TatisLois/docs.openzeppelin.com that referenced this issue Aug 26, 2021
@TatisLois
Copy link
Contributor Author

Thanks for the review @frangio I updated the PR #297 and opened a second one for the babelXRollup #298

frangio added a commit that referenced this issue Sep 7, 2021
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants