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

[Feature] Copy-to-clipboard button in View Query #3571

Merged
merged 2 commits into from
Oct 4, 2017

Conversation

Mogball
Copy link
Contributor

@Mogball Mogball commented Oct 2, 2017

Adds a button that copies the query string:

screen shot 2017-10-02 at 4 39 58 pm

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.141% when pulling 43f179e on Mogball:mogball/feature/copy_query into f8cc05b on apache:master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage remained the same at 70.141% when pulling 43f179e on Mogball:mogball/feature/copy_query into f8cc05b on apache:master.

@mistercrunch
Copy link
Member

We already have some component for this. I'd rather be consistent and always our component, or always use react-copy-to-clipboard . Not 2 different ways to do the same thing...

https://github.com/apache/incubator-superset/blob/master/superset/assets/javascripts/components/CopyToClipboard.jsx

@Mogball Mogball closed this Oct 3, 2017
@Mogball Mogball reopened this Oct 3, 2017
@Mogball
Copy link
Contributor Author

Mogball commented Oct 3, 2017

I modified the existing CopyToClipboard component a bit to make it more robust (wasn't previously working when a modal was open)

@coveralls
Copy link

coveralls commented Oct 3, 2017

Coverage Status

Coverage increased (+0.004%) to 70.145% when pulling ad0a835 on Mogball:mogball/feature/copy_query into f8cc05b on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 70.145% when pulling ad0a835 on Mogball:mogball/feature/copy_query into f8cc05b on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 70.145% when pulling ad0a835 on Mogball:mogball/feature/copy_query into f8cc05b on apache:master.

@Mogball Mogball changed the title Feature: Copy-to-clipboard button in View Query [Feature] Copy-to-clipboard button in View Query Oct 4, 2017
@mistercrunch
Copy link
Member

killingit

@mistercrunch mistercrunch merged commit 7e64f2e into apache:master Oct 4, 2017
@john-bodley
Copy link
Member

john-bodley commented Nov 21, 2017

@mistercrunch @Mogball regrettably it seems the change to copyToClipboard causes an undesirable behavior of including a newline in the copied value. This surfaced when someone copying and pasting a slice shortcut,
screen shot 2017-11-21 at 1 22 24 pm
which gets pasted as (note the empty line):


http://0.0.0.0:8088/r/9

The same is true when copying the query to the clipboard. @Mogball would you be able to address this issue given that your change caused this regression?

@Mogball
Copy link
Contributor Author

Mogball commented Nov 21, 2017

@john-bodley can you make an issue for this and link this PR? Easier to navigate to than digging up a closed PR

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* added copy-to-clipboard button to explore/view query

* modified CopyToClipboard to deselect current before copying
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.2 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants