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

Add copy to clipboard #1624

Merged
merged 4 commits into from Jun 26, 2017
Merged

Add copy to clipboard #1624

merged 4 commits into from Jun 26, 2017

Conversation

deronjohnson
Copy link
Contributor

Addresses #1070 and #1529.

@jseldess For this to work, we need to add some markup (_includes/copy-clipboard.html) to the code snippets, as well as the class "highlight--clipboard" to the code wrapper. However, I'm not sure how we want to implement this, because the code markup is generated by Redcarpet, which I could not find a way to alter. I noticed a few other places in the repository (ex. /install-cockroachdb) where the snippets were hardcoded, it seems to add analytics attributes to code blocks. We might have to take this same approach for snippets that can be copied, although I'm open to other ideas. For now, I added two plain html examples to /start-a-local-cluster so you could see the functionality. The second example strips backslashes followed by new lines, which you discussed in #1529.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jseldess
Copy link
Contributor

jseldess commented Jun 24, 2017

@deronjohnson, thanks for this first attempt. The install page is an exception in the way it hardcodes the html for code samples, as you say, for tracking purposes. That's way too burdensome to do throughout the docs, though, so it seems we do need to find another way.

@benesch and @mjibson, do either of you have ideas?

@deronjohnson, is the requirement that the button be inside the block making this difficult? Would it be easier if it were, say, just beyond the right margin?

@deronjohnson
Copy link
Contributor Author

deronjohnson commented Jun 26, 2017

@jseldess, I was actually able to solve this with CSS. I removed the two examples I hard coded and made the first two code snippets on /start-a-local-cluster copyable. All that is required now is placing {% include copy-clipboard.html %} above a code snippet. You'll notice that the second example doesn't strip the new lines. I wasn't sure if you wanted to concatenate everything to one line only when there were backslashes, or always. Let me know and I can update.

@benesch
Copy link
Contributor

benesch commented Jun 26, 2017

Looks awesome, @deronjohnson!

@jseldess, we want these on essentially every code sample, right? If so, perhaps we can just loop over every <pre> element on document.ready and inject the copy-to-clipboard HTML with JavaScript?

@jseldess
Copy link
Contributor

This is excellent, @deronjohnson!

One request: It's great that you're stripping newlines when there are backslashes! But we seem to also be adding a final newline. When pasted into the terminal, this makes the command immediately execute. Instead, it'd be good for users to be able to review and replace values. So can you strip that final newline from copied code?

@benesch, we actually don't want this feature applied by default. Some code blocks show command responses/outputs. So for now, we'll need the ability to apply the feature only where relevant.

@benesch
Copy link
Contributor

benesch commented Jun 26, 2017

Ah, gotcha. @deronjohnson's solution seems about as terse as it's going to get, in that case!

@deronjohnson
Copy link
Contributor Author

deronjohnson commented Jun 26, 2017

@jseldess, the first example should now paste without the final newline.

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

LGTM now. Just one final tweak. Thanks, @deronjohnson!

~~~ shell
$ cockroach start --insecure \
--host=localhost
~~~

{% include copy-clipboard.html %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the include here, since the block shows a command response that users won't need to copy?

@jseldess
Copy link
Contributor

Thanks, @deronjohnson! Looks great. I'll merge.

@jseldess jseldess merged commit 2ccce9b into cockroachdb:master Jun 26, 2017
@jseldess
Copy link
Contributor

@deronjohnson, just noticed a bug. The copy function seems to remove periods. For example, copying this:

CREATE TABLE bank.accounts (id INT PRIMARY KEY, balance DECIMAL);

ends up as this:

CREATE TABLE bankaccounts (id INT PRIMARY KEY, balance DECIMAL);

Or copying this:

INSERT INTO bank.accounts VALUES (1, 1000.50);

ends up as this:

INSERT INTO bankaccounts VALUES (1, 100050);

Any ideas?

@benesch
Copy link
Contributor

benesch commented Jun 27, 2017

@jseldess: #1631

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 this pull request may close these issues.

None yet

4 participants