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

feat: adding copy button for code snippets #865

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

axel7083
Copy link
Contributor

What does this PR do?

Adding a copy button for the code snippet.

Screenshot / video of UI

copy_button.mp4

What issues does this PR fix or reference?

Fixes #859

How to test this PR?

  • unit tests has been added

@axel7083 axel7083 requested review from benoitf and a team as code owners April 10, 2024 12:00
@slemeur
Copy link
Contributor

slemeur commented Apr 10, 2024

nice!

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
Really great UX improvement 👍

@benoitf
Copy link
Collaborator

benoitf commented Apr 10, 2024

AFAIK it's planned for 1.1 so wondering why we're working on it ?

@axel7083
Copy link
Contributor Author

AFAIK it's planned for 1.1 so wondering why we're working on it ?

I started the code before stevan moved it to 1.1

@benoitf
Copy link
Collaborator

benoitf commented Apr 10, 2024

but we should not tackle enhancements as soon as they appear on the backlog, only after they have been reviewed/ordered

.copyToClipboard(snippet)
.then(() => {
copied = true;
studioClient.telemetryLogUsage('snippet.copy', { language: selectedLanguage, variant: selectedVariant });
Copy link
Contributor

Choose a reason for hiding this comment

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

As there is an event listener on the copy event, I'm wondering if this would lead to a double report on telemetry ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would better to keep only one indeed

@jeffmaury
Copy link
Contributor

The last commit removes the support of Ctrl+C

@axel7083
Copy link
Contributor Author

The last commit removes the support of Ctrl+C

I thought this is what you expected when you mention the duplicates telemetry events ? The last commit does not remove the support of CTRL+C, it removes the telemetry events triggered by this action. But we capture the one from the copy button

@jeffmaury
Copy link
Contributor

The last commit removes the support of Ctrl+C

I thought this is what you expected when you mention the duplicates telemetry events ? The last commit does not remove the support of CTRL+C, it removes the telemetry events triggered by this action. But we capture the one from the copy button

Yes but the request was to capture when snipped was copied into clipboard whatever the original action was

@axel7083
Copy link
Contributor Author

The last commit removes the support of Ctrl+C

I thought this is what you expected when you mention the duplicates telemetry events ? The last commit does not remove the support of CTRL+C, it removes the telemetry events triggered by this action. But we capture the one from the copy button

Yes but the request was to capture when snipped was copied into clipboard whatever the original action was

I can restore the telemetry on the manual ctrl+c if you want and add a differentiator, like 'cpyButton: boolean'

@axel7083 axel7083 force-pushed the feature/copy-button-snippet branch from 16bdb0b to 38f39cf Compare April 11, 2024 08:16
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 requested a review from jeffmaury April 11, 2024 12:55
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@axel7083 axel7083 merged commit e0817a0 into containers:main Apr 11, 2024
4 checks passed
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.

Add copy button to service details client code
5 participants