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

#6680: Model#deleteContent() should not change the type of the trailing block #7270

Merged
merged 12 commits into from
Jun 4, 2020
Merged

#6680: Model#deleteContent() should not change the type of the trailing block #7270

merged 12 commits into from
Jun 4, 2020
+546 −144

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented May 22, 2020 •
edited
Loading

Suggested merge commit message (convention)

Fix (engine): Model#deleteContent() should preserve the properties of more meaningful element. Closes #6680. 

Additional information

@niegowski niegowski marked this pull request as ready for review May 24, 2020 15:01
@niegowski niegowski marked this pull request as draft May 25, 2020 09:16
@niegowski niegowski marked this pull request as ready for review May 25, 2020 16:16
@niegowski
Copy link
Contributor Author

This change replaces this behavior:

With this:

It's using merge operation as original mergeBranches to keep CF compatibility as described here: #4434.

@niegowski
Copy link
Contributor Author

@ckeditor/qa-team could you test it? This is affecting selected elements and also Backspace/Delete behavior.

@FilipTokarski
Copy link
Member

Seems to be ok in all browsers 👍

@niegowski niegowski changed the title #3115: Model#deleteContent() should not change the type of the trailing block #6680: Model#deleteContent() should not change the type of the trailing block May 26, 2020
@niegowski niegowski mentioned this pull request May 26, 2020
Closed
@niegowski niegowski requested a review from jodator May 29, 2020 10:59
jodator
jodator suggested changes Jun 1, 2020
View reviewed changes
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

I would add some more docs and improve test titles.

Other than that. LGTM 👍 .

packages/ckeditor5-engine/src/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/model/utils/deletecontent.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/model/utils/deletecontent.js Outdated Show resolved Hide resolved
@niegowski niegowski requested a review from jodator June 3, 2020 09:04
@jodator jodator merged commit a87b364 into master Jun 4, 2020
@jodator jodator deleted the i/3115 branch June 4, 2020 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@condiom condiom condiom approved these changes

@jodator jodator Awaiting requested review from jodator

Assignees

@jodator jodator

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Backspace should not change the type of the trailing block (probably shouldn't merge it at all)
4 participants
@niegowski @FilipTokarski @jodator @condiom