-
-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
src/components/Clipboard.react.js
Outdated
componentDidMount() { | ||
if (!navigator.clipboard) { | ||
this.setState({hasNavigator: false}); | ||
alert('Copy to clipboard not available with this browser'); |
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 feel like an alert
is too much in this case. Most of the time this component is just a convenience to the user, without it they can select and cmd/ctrl-C. So I wouldn't interrupt their experience with an alert. console.warn
feels sufficient to me, so the developer will know what happened but as far as users are concerned the icon silently fails to render.
Also I don't think hasNavigator
needs to be state
(nor is it quite the right name...) - I bet we could make a module-level const clipboardAPI = navigator.clipboard
, test for it in the constructor if (!clipboardAPI) { console.warn(...) }
and even access it that way clipboardAPI.writeText(text)
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.
Good points - I included both suggestions
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.
Hey @AnnMarieW
Why I am getting "Copy to clipboard not available with this browser" on dash 1.20.0
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.
Hi @AltafHussain4748
The Clipboard
component was not added until V1.21.0, so if you are trying to use it with V1.20.0, I would expect you to see an error like:
module 'dash_core_components' has no attribute 'Clipboard'
If you are using it with V1.21.0 and see the message: Copy to clipboard not available with this browser
, it's because you are using an older browser that doesn't support the Clipboard API. See the browser compatibility list here.
src/components/Clipboard.react.js
Outdated
* The inner text of the `children` prop will be copied to the clipboard. If none, then the text from the | ||
* `value` prop will be copied. | ||
*/ | ||
target_id: PropTypes.string.isRequired, |
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.
Two thoughts about this:
- There may be use cases where the text you want copied doesn't map cleanly to the contents of a specific DOM element. What if we allowed you to omit
target_id
, and if you do that we fall back on another prop (perhaps justtext
) that could be set by a callback if necessary? - pattern-matching callbacks use object IDs, which are then stringified in a specific way when rendered into the DOM. Ideally we should do that for the user (in
getText
), so they can pass the object in astarget_id
without having to know and replicate how we stringify. The code we use for that is here and it's entirely self-contained and expected to be stable, unless @Marc-Andre-Rivet has a clever idea how to share that code I'd probably just duplicate it here.
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.
Regarding the first thought, I added a text
prop as you suggested and also n_clicks
.
Also, if there is no target_id
or text
, the copy icon will just act like a button. This makes it possible to use other copy to clipboard methods in a callback like pandas.DataFrame.to_clipboard
which works great with the DataTable.
On the second point, the stringifyID function worked like a charm :-)
src/components/Clipboard.react.js
Outdated
|
||
getText() { | ||
// get the inner text. If none, use the content of the value param | ||
var text = document.getElementById(this.props.target_id).innerText; |
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.
Use let
instead of var
. But also you should only need to call getElementById
once ie const target = document.getElementById(stringifiedId)
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.
Included in the commit.
src/components/Clipboard.react.js
Outdated
() => { | ||
this.copySuccess(); | ||
}, |
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.
() => { | |
this.copySuccess(); | |
}, | |
this.copySuccess, |
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.
included in next commit
Looking great! Needs a changelog entry, and it would be nice to figure out how to write a test for this. copied_value = dash_dcc.driver.execute_async_script("""
const done = arguments[0];
navigator.clipboard.readText().then(val => { done(val) })
""") or simply copied_value = dash_dcc.driver.execute_async_script("""
navigator.clipboard.readText().then(arguments[0])
""") I worry that this may run afoul of the permissions system though (allowing JS to read from the clipboard can be a security risk) - even if it works locally it may not work on circleCI where the browser runs headless. So it may be better to use keypresses to paste the copied value into a separate element. We have an example of this in the table tests here but that's pretty abstracted - I think it can be something like: dash_dcc.find_element("#paste-into").click()
ActionChains(dash_dcc.driver).key_down(Keys.CONTROL).send_keys("v").key_up(Keys.CONTROL).perform()
dash_dcc.wait_for_text_to_equal("#paste-into", expected_val) For completeness, there's also pyperclip though again I worry a bit about circleCI given pyperclip's comments about linux support. |
…into clipboard � Conflicts: � package-lock.json
…rops, handling object ids
getText() { | ||
// get the inner text. If none, use the content of the value param | ||
const id = this.stringifyId(this.props.target_id); | ||
const target = document.getElementById(id); |
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.
what if you provide a bad target_id
?
document.getElementById('asdf').innerText
> Uncaught TypeError: Cannot read property 'innerText' of null
perhaps add something like if (!target) { return ''; }
immediately after the getElementById
?
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.
Right - that handles the error, but it could be really hard to debug. Is there a way to display a message in the error box?
Another option - don't display the the copySuccess() animation. Do you think that would be enough of a hint to look for an invalid id?
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, that's a good question - no, I don't know of a good way to expose this error to the dash devtools error box - it's happening in an event handler, and React does wrap these so it's possible there's a way we could access them from the renderer. Right now though we don't have a way to do that.
OK, so let's leave it as you have it now - that'll at least prevent copySuccess
and display the error in the js console, and later we can investigate whether there's some generic way to trap event handler errors.
I suppose if you think Uncaught TypeError: Cannot read property 'innerText' of null
is too removed from the issue of not having the right ID, we could also explicitly raise such an error:
if (!target) {
throw new Error("Clipboard copy failed: no element found for target_id " + target_id);
}
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.
Just belatedly passing through and noting this issue for exposing errors in Dash DevTools: plotly/dash#1088
src/components/Clipboard.react.js
Outdated
alert('copy error'); | ||
}); | ||
} else { | ||
this.copySuccess(); |
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.
This is the "just use this icon as a button" case where you specified neither props.text
nor target_id
, right? But as it is we'll also get here if props.text
or the target text is ''
.
I also don't think we want to promote methods like pandas.DataFrame.to_clipboard
- that'll work fine when you're running the app locally, but once deployed it will fail. So seems to me this branch of calling copySuccess
without actually having written anything to the clipboard is not a good idea. I'm not sure though what we should do when there's nothing to copy - put a blank string in the clipboard and still show the check mark? Show an X mark instead (for the same short duration) and don't write anything to the clipboard? I guess I'd lean toward the latter.
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.
OK, I'll take out the "just use the icon as a button."
It's pretty clear that the copy doesn't work when you click on the icon and nothing happens.
And It's probably best to copy nothing to the clipboard in this case. An empty string will clear out the current content.
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.
Looks great! 💃
The test failures (AttributeError: module 'dash_core_components' has no attribute 'Clipboard'
) I believe come from a quirk of how we build packages on CI for PRs that were open before the latest release was made. If you update to the tip of dev
it should work again. Note though that you'll probably have to go in and adjust the CHANGELOG to create a new UNRELEASED section and put this PR into it, just letting git do the merge will leave this PR looking like it was already released!
|
||
elem = dash_dcc.find_element("#paste") | ||
val = elem.get_attribute("value") | ||
assert val == copy_text |
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.
Now both tests are failing on this assertion... either one of these operations is disallowed in the CI environment or it's slower here and the pasted value hasn't shown up yet. I don't think it's slower, because both tests on all Python versions are consistently showing this same error and anyway we do essentially the same stuff in dash-table tests. Same reasoning says paste is not the problem, so the only thing left AFAICT is the clipboard API being blocked.
Here's a stackoverflow post that looks like the same issue: https://stackoverflow.com/questions/53669639/enable-clipboard-in-automated-tests-with-protractor-and-webdriver
Unfortunately the fix looks like it would need to be deep inside dash.testing, I don't see a way to do it locally here.
So if you see these tests passing locally, I'm comfortable skipping them on CI for now and opening an issue in the main dash repo to follow up. You can do this with something like:
@pytest.mark.skipif(
'CIRCLECI' in os.environ,
reason="clipboard API is blocked on CI, see <dash issue url>"
)
def test_clp001...
Hey Everyone, I am getting below issue Property "text" was used with component ID: |
Hi @AltafHussain4748 The prop name You can also see more examples in the announcement of Dash 1.21.0 here. |
The Clipboard component is based on the request in #1009.
The Clipboard component allows the user to copy text from the app to the browser's clipboard by clicking on a copy icon.
The easiest way to trigger the copy is by using the the
target_id
prop. No callback is required!Clicking the copy icon will copy the text to the clipboard and briefly update the icon to show the copy was successful.
![Clipboard](https://user-images.githubusercontent.com/72614349/114420782-7adb5d00-9b69-11eb-8fcd-2500a2305919.gif)
This copies the inner text of components like
dcc.Markdown
andhtml.Table
. For components such asdcc.Textarea
ordcc.Input
it copies the content of the target'svalue
prop. If you would like to customize the text, you can do so by updating the Clipboard'scontent
prop in a callback. This works well with components like the DataTable as shown in the following demo.This demo shows both using Clipboard with and without a callback and 3 ways to style the copy icon.