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

feature: allow to customize the tag of the contentDOMElement #3984

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

nikgraf
Copy link
Contributor

@nikgraf nikgraf commented Apr 25, 2023

Please describe your changes

Added the option contentDOMElementTag to the NodeViewRendererOptions to allow users to provide as the desired element tag they would like to render.

This allows developers to solve the issue with injected divs inside a table when extending the table extension:

How did you accomplish your changes

Added the option contentDOMElementTag

How have you tested your changes

In demos/src/GuideNodeViews/ReactComponentContent/React/Extension.js we tested it using:

return ReactNodeViewRenderer(Component, { contentDOMElementTag: 'main' })

You can see it in action here where we applied the patch via patch-packages: https://github.com/serenity-kit/Serenity/blob/68d7bb29979abf9833116c81f2db27156724a93d/packages/editor/extensions/tableExtension/tableExtension.ts#L15

How can we verify your changes

Reproduce the test?

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

Related issues

@netlify
Copy link

netlify bot commented Apr 25, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 57c3c70
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/6447e3873659810008cc761e
😎 Deploy Preview https://deploy-preview-3984--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 settings.

@sradu
Copy link

sradu commented May 25, 2023

Running into this issue too. In the PR I see that preventContentDOMElement is not there anymore. I'd love to be able to remove it.

@nikgraf
Copy link
Contributor Author

nikgraf commented May 28, 2023

@sradu sorry, forgot to fix the description. I now did.

History: Initially we wanted to have preventContentDOMElement but that led to more and more changes until we stopped following this path and instead replaced it with allowing to define contentDOMElementTag. Minimal change, but fixed our issue.

@sradu
Copy link

sradu commented Jul 21, 2023

Circling back here. The issue is that if the parent element is a grid, and the child nodes are columns, this still breaks.

@nikgraf
Copy link
Contributor Author

nikgraf commented Aug 1, 2023

@sradu oh now

do you have an example? or even better a solution how to solve both cases? would love to see some way to fix the issue

@sradu
Copy link

sradu commented Aug 4, 2023

@nikgraf my very broken fix is:

.node-TYPE [data-node-view-content] > div {
  @apply contents;
}

What is the purpose of that extra div though? Why is it needed?

@nikgraf
Copy link
Contributor Author

nikgraf commented Aug 7, 2023

@sradu interesting trick, need to give it a try.

Initially we tried to remove the div, but realized this is a larger change since plenty of other places reference the div. So this was the lowest impact change to achieve the goal. I hoped this would make it easier to get it merged.

Copy link
Member

@bdbch bdbch left a comment

Choose a reason for hiding this comment

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

LGTM. I'll add docs for this as I'm at it.

@bdbch
Copy link
Member

bdbch commented Aug 18, 2023

@svenadlung you also want to take another look?

@nikgraf
Copy link
Contributor Author

nikgraf commented Aug 18, 2023

awesome thanks 🙌

@bdbch
Copy link
Member

bdbch commented Aug 18, 2023

I'll include this for 2.2.0-rc.0

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.

4 participants