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

Always update fieldView of nestedElement field #365

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

rhystmills
Copy link
Contributor

What does this change?

To be used in conjunction with https://github.com/guardian/flexible-content/pull/4645

This PR makes sure nestedElement fieldViews are always updated, as a workaround for a bug with the zipRoot plugin, that causes the document to be uneditable when an element is deleted between two text elements.

How to test

  1. Use this branch locally with Composer using yalc.
  2. Do all the things suggested in the other PR: https://github.com/guardian/flexible-content/pull/4645

…t hack

to force the fieldViews to update. I've not been able to get to the bottom
of the problem, but merging text elements in the zipRoot plugin after an intermediate element
is deleted causes the document to enter an invalid state that won't be sorted until
'updateFieldViewsFromNode' is called again
@rhystmills rhystmills requested a review from a team as a code owner March 14, 2024 09:51
Copy link
Member

@davidfurey davidfurey left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@rhystmills rhystmills changed the title Always update fieldView of nested element field. This is an unpleasan… Always update fieldView of nestedElement field Mar 14, 2024
@jonathonherbert
Copy link
Contributor

If we've got time, I'd recommend taking a little more time to figure out exactly what's going wrong here, and either

  • dropping the optimisation if it's proving to be too aggressive, rather than working around a special case
  • discovering that something's not right downstream from this update. Top suspects include the node diffing in ProsemirrorFieldView, which tries to update the minimal part of the doc. If this code was the culprit, another approach that would not degrade performance for general documents would be to make updateInnerEditor protected, and override it to remove the diffing from NestedFieldView.

If there's not time for that, then 👍 we can always circle back to fix.

@davidfurey
Copy link
Member

If we've got time, I'd recommend taking a little more time to figure out exactly what's going wrong here, and either

  • dropping the optimisation if it's proving to be too aggressive, rather than working around a special case
  • discovering that something's not right downstream from this update. Top suspects include the node diffing in ProsemirrorFieldView, which tries to update the minimal part of the doc. If this code was the culprit, another approach that would not degrade performance for general documents would be to make updateInnerEditor protected, and override it to remove the diffing from NestedFieldView.

If there's not time for that, then 👍 we can always circle back to fix.

I think our current preference would be to circle back to fix, so that we can get key takeaways out to users

Copy link
Contributor

@Georges-GNM Georges-GNM left a comment

Choose a reason for hiding this comment

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

Have tested with the appropriate composer branch and can confirm we're seeing the desired behaviour; makes sense to me to be using this approach in prod for the moment, considering the significant amount of time that's already been spent on it!

@rhystmills rhystmills merged commit 38d2b63 into main Mar 18, 2024
3 checks passed
@rhystmills rhystmills deleted the rm/zipRoot-workaround branch March 18, 2024 09:26
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

4 participants