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

updateAttributes: update current node only #5154

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

silenius
Copy link

@silenius silenius commented May 15, 2024

There is a bug (?) in updateAttributes, attributes of the node are updated but also all it's parent nodes of the same type. This is problematic with things like nested lists, but also nested paragraphs, etc

The fix updates attributes of the node ("last") only and leaves the parents intact

Changes Overview

updateAttributes update node only (and not its parents)

Implementation Approach

adapt updateAttributes so that it keep tracks of the "last" (current) node

Testing Done

tested locally with bulletList nodes

Verification Steps

try to update an attributes of a nested list for examplectionality of your changes. -->

Checklist

  • I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
  • 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

#3545 #4466

There is a bug (?) in updateAttributes, attributes of the node are updated
but also all it's parent nodes of the same type. This is problematic with
things like nested lists, but also nested paragraphs, etc

The fix updates attributes of the node ("last") only and leaves the
parents intact
Copy link

netlify bot commented May 15, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit cc25b15
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/664ba85e8ba9640009aed7dc
😎 Deploy Preview https://deploy-preview-5154--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.

@nperez0111
Copy link
Contributor

nperez0111 commented May 17, 2024

Appreciate your contribution @silenius but it looks like we need some tests & lint rules resolved for this to be merged

This reverts commit 8a16bbe, reversing
changes made to 90a5a7b.
@silenius
Copy link
Author

Sorry, I pressed the wrong button (:

@nperez0111
Copy link
Contributor

I don't think that is correct either. You were right to pull in the changes from main, it is your changes that are failing the build
@silenius

@silenius
Copy link
Author

I've fixed the lint issue, but I have no idea why it times out in one test..? (unfortunately Cypress is not supported on my OS, FreeBSD, so I can't check at the moment)

@nperez0111
Copy link
Contributor

@silenius if you run the project locally npm run dev you'll see that when you go to: http://localhost:3000/src/Extensions/TextAlign/React/
that if you clear the editor completely, add some new text into the editor (which is what the test does on each run)
then select the content and click the center button that it does not work.

This is also seen on the netlify deployment: https://deploy-preview-5154--tiptap-embed.netlify.app/src/extensions/textalign/react/

So your change broke the ability to center text with the text align extension somehow

Hope this helps @silenius

@nperez0111 nperez0111 added Type: Bug The issue or pullrequest is related to a bug Info: Invalid The issue or pullrequest is invalid labels May 17, 2024
@silenius
Copy link
Author

thanks, it should be fixed :)

@silenius silenius requested a review from nperez0111 May 21, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Info: Invalid The issue or pullrequest is invalid Type: Bug The issue or pullrequest is related to a bug
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

None yet

2 participants