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 option to copy query results directly to clipboard #14889

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Aug 21, 2023

Description

This adds the option to copy query results directly to the clipboard.

Other database UIs (such as BigQuery) provide a similar option and I use it all the time to copy results into github comments, slack threads etc.

Demo:
demo2
(sorry for weird compression, I don't know how to make good screen-recording gifs)

Open Questions:

  • Is it Ok to have copyJSONResultsToClipboard in download.ts, or should it live in its own file under src/utils/
  • Is the phrasing on the dropdown menu clear enough?
  • Should we impose a hidden limitation to prevent crashing the browser with a large output (max 1000 lines, etc)?

Release note

Added option to copy query results in the UI directly to the clipboard

This PR has:

  • been self-reviewed.
  • a release note entry in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@adarshsanjeev
Copy link
Contributor

Hi @SamWheating!

Thanks for the PR! The changes look good and this is a welcome feature. There are a few checkstyle failures in the build due to the imports. Could you please resolve them?

@abhishekagarwal87
Copy link
Contributor

@SamWheating - let us know once you fix the build so we can merge this PR.

@SamWheating
Copy link
Contributor Author

@abhishekagarwal87 thanks for the nudge, I thought I fixed it but it looks like I missed one other check. Should be fixed now but I'll keep an eye on the current CI run.

Copy link
Contributor

@vogievetsky vogievetsky left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. This is a really cool chance. Made some suggestions. LMK what you think.

Copy link
Contributor

@vogievetsky vogievetsky left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the feedback!

@vogievetsky vogievetsky merged commit 73bab2f into apache:master Sep 19, 2023
10 checks passed
@LakshSingla LakshSingla added this to the 28.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants