Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature/clipboard-html #2652

Merged
merged 7 commits into from
Dec 16, 2023
Merged

Conversation

Lxstr
Copy link
Contributor

@Lxstr Lxstr commented Oct 3, 2023

This allows the clipboard to firstly support HTML MIME type. This allows us to copy to clipboard for example, both df.to_string and df.to_html and be able to get the full table on paste where HTML is accepted such as email or other apps. This is optional and independent prop called html_content.

Secondly, the clipboard will now copy on a change to it's content. Previously you could only copy if you clicked on the clipboard item itself. The clipboard item may not be suited for some UIs and so now you can use another component to trigger the copy, such as a button. This behaviour is in line with dcc.Download and many other components.

  • ✅ I have broken down my PR scope into the following TODO tasks
    • ✅ HTML support
    • ✅ Copy on change to content changes as per
  • ✅ I have run the tests locally and they passed. (refer to testing section in [contributing]I think so. I was able to run CI and build.
  • ✅ I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@Lxstr Lxstr requested a review from alexcjohnson as a code owner October 3, 2023 10:59
@alexcjohnson
Copy link
Collaborator

Thanks @Lxstr - two very nice features! I think though that we had better make copy-on-change opt-in, or it could cause unintended copying in existing apps, which may have been created to constantly be updating this content as other things happen in the app. What if we make a new prop like copy_on_change, that can take 3 values:

  • false - default, current behavior
  • true - the behavior you’ve created here
  • ’once’ - if an app wants to trigger a copy programmatically but not by changing the content, maybe because it takes some effort to calculate that content and the author doesn’t want to stash it in some other place waiting to copy. We would immediately reset ’once’ to false after copying the content.

That last option could be added later, but at least the true/false options I think we should do up front, to avoid breaking existing apps.

@Lxstr
Copy link
Contributor Author

Lxstr commented Oct 3, 2023

@alexcjohnson Glad it's helpful! I think that sounds pretty reasonable to me

@Lxstr
Copy link
Contributor Author

Lxstr commented Oct 4, 2023

This may warrant a separate issue but I'm wondering if we are going a bit overboard on the props in general in Dash because we can't directly call methods like copyToClipboard in this case. (or can we? I'm not sure)

I noticed something similar when trying to use the autoSizeAllColumns function in AGGrid. AGGrid has many great methods I would love to directly trigger https://www.ag-grid.com/javascript-data-grid/column-api/

This issue shows the syntax I was originally expecting to be able to use. plotly/dash-ag-grid#99

Ideally, I would like to be able to simply call using the below code rather than using a new prop. I'm guessing passing parameters might be off the table initially, with types going from Python to JS. Passing an empty tuple or True would work initially for many.

@app.callback(
    Output("clipboard", "content"),
    Output("clipboard", "copyToClipboard()"),
    Input("copy_button", "n_clicks"),
)
def copy(_):
    return "Copy me text", ()

@Lxstr Lxstr force-pushed the feature/clipboard-html branch from 9c28972 to 26bf6ab Compare October 30, 2023 04:09
@Lxstr Lxstr force-pushed the feature/clipboard-html branch 2 times, most recently from 29902ad to c119924 Compare December 10, 2023 04:11
@Lxstr
Copy link
Contributor Author

Lxstr commented Dec 11, 2023

@alexcjohnson Hi I've updated the code to trigger copy on a change to n_clicks, that way existing behaviour shouldn't change?

@alexcjohnson
Copy link
Collaborator

@Lxstr that's a great idea! So then we don't need a copy_on_change prop at all, if you want to trigger copy in some other way you just also output a new value to n_clicks, and the component takes that as the signal to copy. I love it!

I'm not sure why so many tests are failing - may just be due to something else we've already fixed, if so updating with the latest changes from the dev branch will fix it. I do see a linting failure, which should be fixable by running npm run format from inside the components/dash-core-components dir.

@Lxstr
Copy link
Contributor Author

Lxstr commented Dec 13, 2023

Thanks @alexcjohnson! I've rebased my branch onto dev and then I ran npm lint, 9 files where changed, all in the tests folder and mostly spacing. I don't understand why so many files would be changed when I've only altered 2? Pre-commit hook still failed after doing this saying

Oh no! 💥 💔 💥
14 files would be reformatted, 172 files would be left unchanged.
npm ERR! code ELIFECYCLE

@Lxstr Lxstr force-pushed the feature/clipboard-html branch from c119924 to 2ffada3 Compare December 13, 2023 23:04
@Lxstr Lxstr force-pushed the feature/clipboard-html branch from 3ce952e to 44639eb Compare December 14, 2023 02:26
@alexcjohnson
Copy link
Collaborator

Oh no! 💥 💔 💥
14 files would be reformatted, 172 files would be left unchanged.

That message is from black - probably a result of a different version than we test with (which is 22.3.0 at the moment, a little out of date so we should fix that!), but that's fine, all you changed was JS so only the eslint and prettier linters are relevant. The lint part of the lint-unit-39 and lint-unit-36 tests passes now :) and the other failures there don't seem related to anything you did. The new test you added is failing though. I haven't looked into why, but does it pass if you run it locally? You should be able to do pytest -k clp003 from in the components/dash-core-components directory.

@T4rk1n would you be able to look at what's going on with the unit part of lint-unit-39 - it works on lint-unit-36, which is weird because the part that fails is in JS, and I'm not sure why that would be different between those two jobs (they're mainly Python 3.6 vs 3.9). Both the passing and failing jobs show a bunch of warnings Critical dependency: the request of a dependency is an expression from Webpack+Mocha and yet the tests still pass in 3.6 but they all hang in 3.9.

@alexcjohnson
Copy link
Collaborator

Hmph weird, not sure what was happening with the unit tests before but they're passing now 🤷 as is your new test 🎉

So now all we need is a changelog entry and this should be good to merge!

@Lxstr
Copy link
Contributor Author

Lxstr commented Dec 16, 2023

Just updated with a changelog. Can I edit the docs somewhere?

@alexcjohnson
Copy link
Collaborator

That's a lot of changes to the changelog - most of them look good but a few I'm not sure about, looks like some of the structure may be changing in unintended ways. I'd rather this PR just add its own line, and we can take a second PR to clean up the changelog formatting.

Dash docs currently live in a private repo, unfortunately. The new docstrings will be picked up automatically but if you want to add new examples, easiest would be to just describe the example here and we can incorporate it into the docs (cc @LiamConnors)

@Lxstr
Copy link
Contributor Author

Lxstr commented Dec 16, 2023

OOPS sorry I must have accidentally saved with formatting, I'll revert now.

For the docs I think what the docstring picks up should be fine, people will see the new prop and also the n_clicks behaviour is what I was anticipating originally anyway

@Lxstr Lxstr force-pushed the feature/clipboard-html branch from 34e939a to 9469c58 Compare December 16, 2023 12:22
@Lxstr
Copy link
Contributor Author

Lxstr commented Dec 16, 2023

Just updated now. Thanks for all the help

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Excellent, thanks so much! I'll merge once the tests pass 🎉

@alexcjohnson alexcjohnson merged commit 7527383 into plotly:dev Dec 16, 2023
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants