-
Notifications
You must be signed in to change notification settings - Fork 178
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
Preserve the original editor observer #1006
base: main
Are you sure you want to change the base?
Conversation
E.g., messages from Yandex metrics
✅ Deploy Preview for getmavo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -426,7 +426,14 @@ var _ = Mavo.Primitive = class Primitive extends Mavo.Node { | |||
} | |||
|
|||
for (let primitive of nodes) { | |||
primitive.originalEditorUpdated({force: true}); |
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.
Was this line removed accidentally or we genuinely don't need it anymore?
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.
My investigation shows that this line is the one that was added in the first bad commit and caused the regression. The only thing I did was revert the previous version of this code block.
I think I understand why this line was present—we get the same result as when we have the two added lines. However, to add to this, we destroy the original editor observer and replace it with the observer of the clone of the original editor. It works fine if the element is not in the edit mode initially. But when it is, we clone the original editor before Mavo can update expressions and handle the original editor mutations.
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.
It looks like we need this line since, without it, we break another test with dynamic selects. I should continue working on a proper fix.
@@ -441,7 +448,7 @@ var _ = Mavo.Primitive = class Primitive extends Mavo.Node { | |||
|
|||
let editor = this.editor ?? this.originalEditor; | |||
|
|||
if (editor?.matches("select:not(.mv-options-select")) { | |||
if (editor?.matches("select:not(.mv-options-select)")) { |
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.
Whoa, how did this go undetected all this time?!
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.
We have it not so long as you might imagine—not longer than we have mv-options
. 😉 We couldn't detect it thanks to the browser forgiveness, I believe. I checked in the console—both variants return the same result.
Fixes regression #996.
The main issue is that we lose the original editor observer when we force the original editor update, so the changes are not picked up when the original editor is updated via an expression.