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: Prevent unneccessary server calls when inline editor is unchanged #3833

Conversation

grebaldi
Copy link
Contributor

fixes: #3832

Straight-forward fix: This PR adds a check to prevent calls to the change API when the inline editor is unchanged.

@grebaldi grebaldi requested a review from mhsdesign July 17, 2024 16:46
@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.3 labels Jul 17, 2024
@@ -74,6 +74,14 @@ export default ({store, globalRegistry, nodeTypesRegistry, inlineEditorRegistry,
actions.Changes.persistChanges([change])
),
onChange: value => {
const node = selectors.CR.Nodes.byContextPathSelector(contextPath)(store.getState());
Copy link
Member

Choose a reason for hiding this comment

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

i though hey why not use the transientValues ... but we only calculate this dirty state for the inspector ... so i guess this is the way ;) ...

a possibly 'cleaner'? in terms of not - depending - on the node - here - and doing it before onChange called - alternative could be to track what the ckeditor changed currently via change:data and depending on this invoke the callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While probably possible, I wouldn't regard it as cleaner. Tracking CKEditor state is a whole can of worms in its own right, especially since we are only interested in changes that happened since the last time the property was focused.

Checking for actual changes before doing the remote call on the other hand always makes sense - and renders all tracking on CKEditor level moot. I was actually surprised that we didn't do that already 😅

Copy link
Member

Choose a reason for hiding this comment

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

okay you got me mostly convinced ... i just dislike callback - in this case the onChange - being fired when they simply shouldnt.
Nothing changed if we defocus a not - dirty - ckeditor (maybe ckeditor even has a flag for this?) so thats what i was referring to being "cleaner".

But still seb approved as well so feel free to merge ;)

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thank you so much for investigating so quickly ;) It makes totally sense that it fails currently because of this new line: https://github.com/neos/neos-ui/pull/3751/files#diff-f2abb91e96ac7035f92ab489c0b6393a31683dadd9f5d3946e4cecab5b9e5373R62

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

LGBR

I assume the strict comparison should work fine.

@grebaldi grebaldi merged commit fda27d7 into neos:8.3 Jul 18, 2024
9 checks passed
mhsdesign added a commit that referenced this pull request Sep 5, 2024
…is unchanged"

This reverts commit 180906d.

The change introduced with pr #3833 is not needed when onChange is only triggerd on real actual changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Simply clicking into a ckeditor text field and afterwards selecting something else triggers a change
3 participants