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 child element decorations #4910

Merged

Conversation

jasonphillips
Copy link
Contributor

Description
Due to a regression, decorations that cross child elements were not properly passed to those elements for rendering; the offsets from the anchor and node were being applied to every child node, rather than the specific intersection. This PR ensures that the child elements always receive the intersecting range of the decoration.

Issue
Fixes: #4909

Example
Given this value, where some elments are nested under a parent element:

[
  {
    type: "parent",
    children: [
      { type: "block", children: [{ text: "foo" }] },
      { type: "block", children: [{ text: "bar" }] },
      { type: "block", children: [{ text: "baz" }] }
    ]
  }
]

And this return from decorateNode on the parent element above:

[
    {
        anchor: { path: [0, 1, 0], offset: 1 },  // from b(ar
        focus: { path: [0, 2, 0], offset: 2 },   // to ba)z
        bold: true
    }
]

The decorations passed down to the child elements will be only their intersecting range, so that the decoration is applied correctly across the children.

From the linked issue, see this image of the regression (this PR seeks to restore prior behavior shown on 0.66 here):
image

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 23, 2022

🦋 Changeset detected

Latest commit: 898cd87

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

@BitPhinix
Copy link
Contributor

🚀

@jasonphillips
Copy link
Contributor Author

The regression I'm fixing was caused by #4876 - which I should have tagged above for context.

Copy link
Contributor

@nemanja-tosic nemanja-tosic left a comment

Choose a reason for hiding this comment

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

Good catch, and good PR.

I've left a couple of questions for consideration, and also if you could as part of this PR please remove const decorate = useDecorate() on line 36 (and the import for useDecorate) as it's not used anymore.

packages/slate-react/test/index.spec.tsx Outdated Show resolved Hide resolved
@dylans dylans merged commit 2a8d86f 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
* fix slate-react handling of nested element decorations

* chore: add changeset

* changes from review
jasonphillips added a commit to jasonphillips/slate that referenced this pull request May 25, 2022
dylans pushed a commit that referenced this pull request May 25, 2022
* Revert "Fix child element decorations (#4910)"

This reverts commit 2a8d86f.

* Revert "Fix stale decorations (#4876)"

This reverts commit 1b205c0.

* chore: add changeset
@github-actions github-actions bot mentioned this pull request May 25, 2022
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.

Decorations incorrectly rendered across child elements
4 participants