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 off by one error for images (and probably other atomic nodes) #5327

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Nantris
Copy link
Contributor

@Nantris Nantris commented Jul 11, 2024

Changes Overview

Fixes NodePos miscalculation for images

Implementation Approach

Reverts the spirit, but not letter, of the approach that was used before the regression occurred: https://github.com/ueberdosis/tiptap/pull/5038/files#diff-deab15d8918868fc71b9ad8ee3afa808f6b4be07c48e915fd0aed512493fd90cL139

Testing Done

TESTING WAS NOT DONE

Verification Steps

Additional Notes

@masylum your help confirming this PR works as intended would be greatly appreciated

Checklist

  • I have created a changeset for this PR if necessary.
  • My changes do not break the library.
  • I have added tests where applicable.
  • I have followed the project guidelines.
  • I have fixed any lint issues.

Related Issues

#5314

Copy link

changeset-bot bot commented Jul 11, 2024

⚠️ No Changeset found

Latest commit: 536a55f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 536a55f
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/668f57feb5eb390008875504
😎 Deploy Preview https://deploy-preview-5327--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Nantris
Copy link
Contributor Author

Nantris commented Jul 11, 2024

@svenadlung could you take a look at this? I'm not sure if there was a good reason the +1 was removed to begin with or if it was just an oversight. (https://github.com/ueberdosis/tiptap/pull/5038/files#diff-deab15d8918868fc71b9ad8ee3afa808f6b4be07c48e915fd0aed512493fd90cL139)

I'm also unsure whether comparing based on isBlock or isAtom is better (if either one is.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage open
Development

Successfully merging this pull request may close these issues.

1 participant