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

Fix NODE_TO_KEY correction for split_node and merge_node #4901

Merged
merged 4 commits into from
Mar 25, 2022

Conversation

bryanph
Copy link
Contributor

@bryanph bryanph commented Mar 20, 2022

Description
Fixes #4900

Issue
Fixes: #4900

Example
The logic in with-react was not resetting the node keys for the right nodes for split_node and merge_node operations. This fixes that so that on split_node the node being split does not get remounted and on merge_node the node being merged into also does not get remounted.

Context
See #4900

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 Mar 20, 2022

🦋 Changeset detected

Latest commit: c242b0a

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

This PR includes changesets to release 1 package
Name Type
slate-react 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.

Thanks! I’d love to see a test for this though I know that’s not easy yet. Please add a changeset when you get a chance.

@bryanph
Copy link
Contributor Author

bryanph commented Mar 21, 2022

@dylans yeah a test here would be nice indeed, will see what I can do. For a test I'd have to check whether a component remounted or not, I don't think cypress can do that. Or can go more basic and test the matches array returned here is correct for some cases.

@nemanja-tosic
Copy link
Contributor

@bryanph don't know about cypress, but here's a react test renderer version that fails against previous version but works with the PR:

it('should not unmount', async () => {
  const editor = withReact(createEditor())
  const value = [{ children: [{ text: 'hello' }] }]
  const mounts = jest.fn<void, [Element]>()

  let el: ReactTestRenderer

  act(() => {
    el = create(
      <Slate editor={editor} value={value} onChange={() => {}}>
        <DefaultEditable
          renderElement={({ element, children }) => {
            useEffect(() => mounts(element), [])

            return children
          }}
        />
      </Slate>,
      { createNodeMock }
    )
  })

  // slate updates at next tick, so we need this to be async
  await act(async () =>
    Transforms.splitNodes(editor, { at: { path: [0, 0], offset: 2 } })
  )

  expect(mounts).toHaveBeenCalledTimes(2)

  // TODO: assertion against something specific to elements
})

@bryanph
Copy link
Contributor Author

bryanph commented Mar 24, 2022

@nemanja-tosic awesome, thanks! I pushed two tests for the particular cases being fixed here based on your example here. Let me know if there's something else I can assert here.

@dylans dylans merged commit 5ef346f into ianstormtaylor:main Mar 25, 2022
@github-actions github-actions bot mentioned this pull request Mar 25, 2022
DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
…lor#4901)

* Fix NODE_TO_KEY correction for split_node and merge_node

* fix lint

* add changeset

* Add NODE_TO_KEY tests for number of mounts for split_node and merge_node
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.

Slate unmounts unchanged elements on split_node, merge_node
3 participants