-
Notifications
You must be signed in to change notification settings - Fork 32
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
fixed copy code block bug #448
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Left a suggestion for your consideration.
source/cloud/gcp/gke.md
Outdated
$ gcloud container clusters create rapids-gpu-kubeflow \ | ||
--accelerator type=nvidia-tesla-a100,count=2 --machine-type a2-highgpu-2g \ | ||
--zone us-central1-c --release-channel stable | ||
$ $ gcloud container clusters create rapids-gpu-kubeflow --accelerator type=nvidia-tesla-a100,count=2 --machine-type a2-highgpu-2g --zone us-central1-c --release-channel stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ $ gcloud container clusters create rapids-gpu-kubeflow --accelerator type=nvidia-tesla-a100,count=2 --machine-type a2-highgpu-2g --zone us-central1-c --release-channel stable | |
$ gcloud container clusters create rapids-gpu-kubeflow --accelerator type=nvidia-tesla-a100,count=2 --machine-type a2-highgpu-2g --zone us-central1-c --release-channel stable |
I don't think we want multiple leading $
, can you please remove one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually... could you test something? Could you push a change here to remove ALL the leading $
in this code block, and put back the multi-line break? Then we could use the preview at https://rapids-deployment--448.org.readthedocs.build/en/448/cloud/gcp/gke/ to test if the copy button does the expected thing in that case, and could consider just dropping the leading $
.
I really think that'd be preferable if it does work... look how much information is hidden by putting this on 1 line:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay I tested it on a local server and it works! I'll push those changes right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great, glad that worked! I just tried on the docs preview and also confirmed it works as expected. And I think it's great that it now renders with the entire command visible:
Could you please open an issue documenting the work to remove these leading $
from all the shell blocks across this repo?
I've always found them a little annoying anyway (because you have to manually exclude them when copying in a place that doesn't have the copy-button logic that filters them out), and now we have a specific functional reason to prefer not having them.
Don't actually do that implementation work though (except for other multi-line cases like this one where it's necessary to fix them), until @jacobtomlinson gives us his opinion on the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few thoughts on this, @melodywang060 if you could open the issue that James suggested then we can discuss it over there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, I'll open it up right now 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New issue and documentation is here: #452
Problem
Currently the entirety of the multiline command isn't copied over when users hit the
copy
code block button (only the first line)Quick Fix
Turning the multiline block into a single line
Pros
More convenient for users to copy and paste
Cons
May be harder to read and interpret each of the configurations in the arguments