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

Allow to disable new line padding on paste for specific elements #17068

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Sep 10, 2024

Suggested merge commit message (convention)

Fix (clipboard): Pasting an inline widget as plain text no longer inserts additional empty lines before and after pasted widget. Closes #17052

MAJOR BREAKING CHANGE (clipboard): Pasting an inline widget as plain text no longer inserts additional empty lines before and after pasted widget.


Additional information

Not sure if proposed solution is the correct one. How can we determine model element schema attributes based on view elements from clipboard content?

Before:

before-new-line-fix-2024-09-10_08.51.47.mp4

After:

after-new-line-fix-2024-09-10_08.50.11.mp4

@Witoso
Copy link
Member

Witoso commented Sep 10, 2024

Why this works on our merge fields and doesn't on custom written placeholders? Maybe we lack some abstraction?

@Mati365
Copy link
Member Author

Mati365 commented Sep 10, 2024

@Witoso From what I can see merge tags use dataPipeline:transparentRendering to handle this, but in our case it's not working properly. It's not upcasting placeholders and keeps raw text after setting this attribute on placeholder element specified in example.

@Dumluregn Do you know why this happens? Can we use dataPipeline:transparentRendering without breaking upcasting?

Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

The newLinePadding() helper is unfortunately based on the assumption that the view ContainerElement is a block element and this is not a valid assumption. Maybe we could use DomConverter#blockElements as a list of view elements which should be considered a block?

I'd suggest not adding another custom property.

@Mati365
Copy link
Member Author

Mati365 commented Sep 11, 2024

@niegowski Isn't it breaking change? I'm not sure what should API look like, because we have plenty of downcasts which do not determine which view element is a block element and which not.

ahh, now I get it. I was thinking that we want to add new blockElements property. nvm

@Mati365 Mati365 force-pushed the ck/16923 branch 3 times, most recently from 4d2e0fa to 66bf853 Compare September 12, 2024 11:56
@Mati365
Copy link
Member Author

Mati365 commented Sep 12, 2024

@niegowski Looks like it works 🎉

@Mati365 Mati365 requested a review from niegowski September 12, 2024 11:57
@Mati365 Mati365 requested a review from niegowski September 20, 2024 12:04
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

The code looks ok but the breaking change description must be updated to reflect the code change. The signature of that helper changed and since it's a public helper it could be used in some external integrations.

@Witoso
Copy link
Member

Witoso commented Sep 20, 2024

Please, let's plan this BC together for some specific release. Don't merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting an inline widget as plain text inserts additional empty lines before and after widget text
3 participants