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

Take previously added/removed nodes into account when fetching child … #5396

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

Moerphy
Copy link
Contributor

@Moerphy Moerphy commented Apr 18, 2023

Description
When when a node has two child nodes that would violate the core constraints (i.e. two inline nodes as children of the Editor) this can cause other nodes to be deleted that do not violate any core constraints.

Example
Input:

<editor>
    <inline>one</inline>
    <block>two</block>
    <inline>three</inline>
    <block>four</block>
</editor>

Normalized result before this change:

<editor>
    <block>four</block>
</editor>

Normalized result after this change:

<editor>
    <block>two</block>
    <block>four</block>
</editor>

See adapted unit tests for more examples.

Context
Disclaimer: I only know very little about Slates internal, so my understanding here might not be 100% correct.

Looks to me like the current logic for iterating children in the normalization uses two sets of variables node & i and currentNode & n. I think node & i are referring to the state of the editor when normalizeNode was entered and currentNode& n are referring the "current" state, taking into account transforms from an earlier iteration.
The transforms that are executed when a constraint violation is detected are always using n to construct the path to the transformed node, but the constraint was checked on the child node at index i instead, which leads to the wrong nodes being transformed.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Apr 18, 2023

🦋 Changeset detected

Latest commit: 8640c58

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me @Moerphy . We'll land it. If it breaks something else we can always revert.

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

2 participants