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

Bugfix/5650 inserting text uses model data not current view #5651

Conversation

Wayne-Mather
Copy link
Contributor

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

@@ -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 ?? '';
Copy link
Contributor

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.

Copy link
Contributor Author

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);
Copy link
Contributor

@bgrand-ch bgrand-ch Jan 31, 2024

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@artf
Copy link
Member

artf commented Feb 11, 2024

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. textView.insertComponent({...}, { useDomContent: true }))

@Wayne-Mather
Copy link
Contributor Author

@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

@artf artf merged commit 9ee52e6 into GrapesJS:dev Feb 19, 2024
2 checks passed
@artf
Copy link
Member

artf commented Feb 19, 2024

Thanks @Wayne-Mather

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.

None yet

3 participants