-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Bugfix/5650 inserting text uses model data not current view #5651
Bugfix/5650 inserting text uses model data not current view #5651
Conversation
@@ -182,13 +182,13 @@ export default class ComponentTextView extends ComponentView<ComponentText> { | |||
const offset = range.startOffset; | |||
const textModel = getComponentModel(textNode); | |||
const newCmps: (ComponentDefinition | Component)[] = []; | |||
|
|||
let data = textNode.textContent ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: const data = textNode.textContent || '';
, because the data
is not updated afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (textModel && textModel.is?.('textnode')) { | ||
const cmps = textModel.collection; | ||
cmps.forEach(cmp => { | ||
if (cmp === textModel) { | ||
const type = 'textnode'; | ||
const cnt = cmp.content; | ||
const cnt = (data = '' ? cmp.content : data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important: use equality operator (==
) or strict equality operator (===
)
suggestion: const cnt = data || cmp.content;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This might lead to unexpected behavior as it's not always the case I want to use the DOM content. I'd accept this only behind an additional option flag (eg. |
…n the options object
@artf I have wrapped the code to test if that property exists and if it's true. That way content is only replaced if insertComponent(xx, { useDomContent: true }) is used |
Thanks @Wayne-Mather |
This is my proposed solution for the bug 5650 when inserting text into a text node that has not synchronised back to the model yet.
BUG: When inserting text when component not synced, the text is overwritten from the existing model