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

fix(extension-table):Clipboard Issue: Unwanted Text Included When Pas… #5372

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wallace-yang
Copy link

fix(extension-table):Clipboard Issue: Unwanted Text Included When Pasting Table Columns Outside of the Editor (#5086)

Changes Overview

Modify the clipboardTextSerializer.ts file to ensure that the clipboardText is correct after copying a cell.

Implementation Approach

Testing Done

Copy a cell from the table and paste it outside of the

Verification Steps

Additional Notes

Checklist

  • [] I have created a changeset for this PR if necessary.
  • My changes do not break the library.
  • [] I have added tests where applicable.
  • I have followed the project guidelines.
  • I have fixed any lint issues.

Related Issues

#5086

Copy link

changeset-bot bot commented Jul 22, 2024

⚠️ No Changeset found

Latest commit: 5761023

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 5761023
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/669dcf5e2203c300080e7a38
😎 Deploy Preview https://deploy-preview-5372--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines +13 to +15
interface _Fragment extends Fragment {
content: Node[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be extending prosemirror types. If it says it does not have a content property then it probably is not meant to be accessed

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we should be extending prosemirror types. If it says it does not have a content property then it probably is not meant to be accessed

However, there is indeed a content property, and the type of this object is _Fragment, with its parent being _Slice. I couldn't find where they originated from; neither tipTap nor ProseMirror defines them. So I added _Fragment myself. Do you have any good solutions to

Copy link
Contributor

Choose a reason for hiding this comment

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

content is not meant to exposed though because it is an internal property in prosemirror: https://github.com/ProseMirror/prosemirror-model/blob/cde085e345b6815c54af9c386feb73bce3ad41ee/src/fragment.ts#L17-L18

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a forEach method which allows iterating over each item: https://github.com/ProseMirror/prosemirror-model/blob/cde085e345b6815c54af9c386feb73bce3ad41ee/src/fragment.ts#L168

But I'm not sure about this approach in general

Copy link
Author

Choose a reason for hiding this comment

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

There is a forEach method which allows iterating over each item: https://github.com/ProseMirror/prosemirror-model/blob/cde085e345b6815c54af9c386feb73bce3ad41ee/src/fragment.ts#L168

But I'm not sure about this approach in general

Thank you for your suggestion. I'll give it a try later.

Comment on lines +41 to +54
if (isCellSelection(view.state.selection)) {
const contentArray = (content.content as _Fragment).content
let result = ''

contentArray.forEach((tableRow:Node) => {
const cellArr = (tableRow.content as _Fragment).content

cellArr.forEach((cell:Node, cellIndex:Number) => {
result = `${result} ${cell.textContent} ${Number(cellIndex) === cellArr.length - 1 ? '\n' : ''}`
})
})
return result
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like the correct place for this, for example, where is the logic for adding these new lines for things like list items that you'd expect a newline between too?

Maybe they need custom renderText methods?

Copy link
Author

Choose a reason for hiding this comment

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

list items

I didn't take into account the situation where the table contains list items. I will reconsider

Copy link
Contributor

Choose a reason for hiding this comment

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

Not only when it contains list items, but any content that can be broken into lines.

I think that you need a custom renderText method for table rows that allow you to insert line breaks or something.

But I don't completely understand what the expected outcome should be here, a table is fundamentally very different from plain text so it would never fully be understood

Copy link
Author

Choose a reason for hiding this comment

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

image
As far as we are concerned, the expectation is as follows.
1 2
4 5

The result of Prosemirror is like this.
1

2

4

5

I will change the rules to be the same as ProseMirror, and then reconsider whether to add a custom renderText method
Thank you for your suggestion.

@nperez0111 nperez0111 added the Info: Blocked The issue or pullrequest is blocked by another issue or pullrequest label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Info: Blocked The issue or pullrequest is blocked by another issue or pullrequest
Projects
Status: Triage open
Development

Successfully merging this pull request may close these issues.

2 participants