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: apply #5147 fix to marks and nodes #5156

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

nperez0111
Copy link
Contributor

@nperez0111 nperez0111 commented May 15, 2024

Changes Overview

As a follow up to #5147
In my previous PR, @634750802 reminded me that Nodes & Marks both have the configure method as well and have almost the exact same logic as extensions. So I applied the change to Nodes & Marks as well

Implementation Approach

See $5147

Testing Done

Ran all tests, everything looks good

Verification Steps

See above

Additional Notes

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

Copy link

netlify bot commented May 15, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit dee3946
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66711d635eec5f00086e81d5
😎 Deploy Preview https://deploy-preview-5156--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.

Comment on lines 50 to 68
// addProseMirrorPlugins() {
// console.log('custom', this, this.parent)
// return [
// ...this.parent(),
// ]
// },
})
.configure({ lowlight }),
.configure({ lowlight })
.extend({
addProseMirrorPlugins() {
return [
...this.parent(),
]
},
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need to add more tests for this. Just wanted it out of my mind

packages/core/src/Mark.ts Outdated Show resolved Hide resolved
packages/core/src/Node.ts Outdated Show resolved Hide resolved
Comment on lines 459 to 465
const extension = this.extend({
addOptions() {
return mergeDeep(this.parent?.() || {}, options) as Options
},
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the secret sauce, a configure is just a short way of expressing additional options to be applied

packages/core/src/Node.ts Outdated Show resolved Hide resolved
@nperez0111 nperez0111 force-pushed the issue/configure-parent-node-mark branch from 62e5c1f to daa3c66 Compare June 18, 2024 04:53
@nperez0111 nperez0111 force-pushed the issue/configure-parent-node-mark branch from a7e02b3 to dee3946 Compare June 18, 2024 05:38
@nperez0111 nperez0111 merged commit c540c7d into develop Jun 18, 2024
14 checks passed
@nperez0111 nperez0111 deleted the issue/configure-parent-node-mark branch June 18, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

1 participant