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.

Sign up for GitHub

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

Jump to bottom

Enable copying text to clipboard for tables with cell_selectable=False #2114

Merged
merged 7 commits into from
Jul 7, 2022
Merged

Enable copying text to clipboard for tables with cell_selectable=False #2114

merged 7 commits into from
Jul 7, 2022

Conversation

benedikt-budig
Copy link
Contributor

@benedikt-budig benedikt-budig commented Jul 1, 2022 •
edited
Loading

This PR makes sure that for DataTables with cell_selectable=False, the text content of a cell can be selected and copied to the clipboard as is normally handled by the browser.

It fixes issue #1978; the underlying problem has been open for quite some time (see also plotly/dash-table#822).

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • 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

@benedikt-budig benedikt-budig mentioned this pull request Jul 1, 2022
Open
@benedikt-budig
Copy link
Contributor Author

benedikt-budig commented Jul 1, 2022 •
edited
Loading

The percy/dash check seems to be failing because the baseline snapshot still has a datepicker that shows June (and not July). Are the baseline snapshots updated automatically after some time?

alexcjohnson
alexcjohnson reviewed Jul 5, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

@benedikt-budig thanks for the PR! Looks great, and don't worry about Percy, I'll approve those when we're ready.

The one thing we should do is add a test that shows that the bug is fixed. This should be a good starting point (and this test I believe verifies that the normal cell_selectable=True copy still works correctly with your fix?):

dash/components/dash-table/tests/selenium/test_basic_copy_paste.py

Lines 154 to 174 in 565de53

def test_tbcp004_copy_9_and_10(test):
test.start_server(get_app())
source = test.table("table")
target = test.table("table2")
source.cell(9, 0).click()
with test.hold(Keys.SHIFT):
ActionChains(test.driver).send_keys(Keys.DOWN).perform()
test.copy()
target.cell(0, 0).click()
test.paste()
for row in range(2):
for col in range(1):
assert (
target.cell(row, col).get_text() == source.cell(row + 9, col).get_text()
)
assert test.get_log_errors() == []

@benedikt-budig
Copy link
Contributor Author

@alexcjohnson thanks a lot for your feedback! In the meantime, I have:

  • added a unit test covering the fixed issue, and
  • slightly extended Contents to add a new CSS class to table cells indicating whether they are selectable or not.

However, there are now some long_callback tests failing. It seems to me that these tests are operating on the edge of the 10 seconds timeout and I'm not sure if my changes have actually something to do with them failing. Could you help me once again?

@T4rk1n
Copy link
Contributor

T4rk1n commented Jul 7, 2022

However, there are now some long_callback tests failing. It seems to me that these tests are operating on the edge of the 10 seconds timeout and I'm not sure if my changes have actually something to do with them failing. Could you help me once again?

@benedikt-budig There was a lock in the tests for the older implementation of long_callbacks that made the tests flaky, I removed the lock and it should be fine now. You can merge with dev and the tests should pass.

alexcjohnson
alexcjohnson approved these changes Jul 7, 2022
View reviewed changes
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.

💃 Beautiful - great call adding the selectable class so we can keep background-color: transparent; in that case. And nice job on the test!

Will merge once tests pass, let's hope merging dev will fix them 🤞

@alexcjohnson alexcjohnson merged commit 88d21cd into plotly:dev Jul 7, 2022
@benedikt-budig
Copy link
Contributor Author

@alexcjohnson @T4rk1n awesome, thanks for your help and for merging this! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benedikt-budig @alexcjohnson @T4rk1n