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

Merged
merged 13 commits into from
Jun 7, 2024

Conversation

silenius
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor Author

thanks, it should be fixed :)

@silenius silenius requested a review from nperez0111 May 21, 2024 09:12
silenius added a commit to silenius/amnesia_admin that referenced this pull request Jun 4, 2024
@nperez0111 nperez0111 merged commit a52118c into ueberdosis:main Jun 7, 2024
14 checks passed
@nperez0111
Copy link
Contributor

Appreciate your contribution here, it will go into the next release. Might take a while, so I will make a beta that includes it soon

@svilen-ivanov-kubit
Copy link

Hi, I think this change caused the following problem: #5254

@nperez0111
Copy link
Contributor

Hi, I think this change caused the following problem: #5254

Thanks for your feedback. I will write a test for this & see if I can resolve it or revert it

@nperez0111
Copy link
Contributor

Sorry @silenius I did not get around to handling this. If you have an idea on how to support this, that would very much be appreciated. I'm trying to get this new release out and so I had to revert it

@silenius
Copy link
Contributor Author

Hi @nperez0111,

Sorry for breakage, I'll try to find a little bit of time to review it.. but it could be reverted in the meantime

@nperez0111
Copy link
Contributor

Thanks for your contribution in the first place!

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: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants