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(html): allow caller to specify a document implementation in generateHTML() #4047

Merged
merged 2 commits into from
Aug 18, 2023

Conversation

srsudar
Copy link
Contributor

@srsudar srsudar commented May 15, 2023

Please describe your changes

Allows callers of the generateHTML() fn to pass their own document implementation. This is necessary because renderHTML() can return a DOMElement as a DOMOutputSpec. However, the renderHTML() method on nodes doesn't provide access to the document used in the render (at least as far as I can tell).

TipTap assumes that the document passed to DOMSerializer can always be the document that comes from zeed-dom. However, zeed-dom appears to be incompatible with jsdom. Without the change in this PR, any node that returns an HTMLElement created via jsdom is serialized as the string {}.

How did you accomplish your changes

I added an option to generateHTML() that allows callers to specify a document alternative to zeed-dom.

How have you tested your changes

I have this implementation working in another project. I've not yet tested it in this repo--as my first PR, I want to see where the tests fail an then follow up.

How can we verify your changes

TBD

Remarks

An alternative might be to add the document being used to serialize the fragment to the renderHTML() method, so that a node could do something like:

renderHTML(params): {
  return params.document.createElement('div');
}

As it stands with this PR, they can just use:

renderHTML(params): {
  // this `document` must be made available globally by other
  // set up code.
  return document.createElement('div');
}

That approach would be a little less magical, and perhaps easier to use on the part of the caller. However, the approach I've taken works for my project and I'd like feedback from the maintainers before going on that more involved API change.

Checklist

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

Related issues

[add a link to the related issues here]

@netlify
Copy link

netlify bot commented May 15, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 7881dea
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/64a9f46ae10fbb000836ebed
😎 Deploy Preview https://deploy-preview-4047--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.

@srsudar srsudar changed the title allow caller to specify a document implementation allow caller to specify a document implementation in generateHTML() May 15, 2023
@srsudar srsudar changed the title allow caller to specify a document implementation in generateHTML() allow caller to specify a document implementation in generateHTML() May 15, 2023
@bdbch
Copy link
Contributor

bdbch commented Jul 7, 2023

Nice idea. It seems like Netlify is crashing though because of this:

3:37:41 PM: > tsc --project tsconfig.base.json --noEmit && tsc --project tsconfig.react.json --noEmit && tsc --project tsconfig.vue-2.json --noEmit && tsc --project tsconfig.vue-3.json --noEmit
3:37:44 PM: ../packages/html/src/getHTMLFromFragment.ts(9,18): error TS2448: Block-scoped variable 'document' used before its declaration.
3:37:44 PM: ../packages/html/src/getHTMLFromFragment.ts(9,18): error TS2454: Variable 'document' is used before being assigned.
3:37:44 PM: ../packages/html/src/getHTMLFromFragment.ts(10,128): error TS2345: Argument of type 'VElement' is not assignable to parameter of type 'HTMLElement | DocumentFragment | undefined'.
3:37:44 PM:   Type 'VElement' is missing the following properties from type 'HTMLElement': accessKey, accessKeyLabel, autocapitalize, dir, and 239 more.
3:37:44 PM: npm ERR! Lifecycle script `ts` failed with error

Could you take a look?

@srsudar
Copy link
Contributor Author

srsudar commented Jul 8, 2023

Well that was a silly error. I can't get that command to run locally for some reason. Letting CI run again...

@srsudar
Copy link
Contributor Author

srsudar commented Aug 17, 2023

@bdbch done, I think...

Copy link
Contributor

@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 thought about adding this to the documentation, but I think we don't even have a documentation page that references getHTMLFromFragment yet.

@svenadlung could you take a second look? Otherwise this one would be fine for me.

@bdbch bdbch changed the title allow caller to specify a document implementation in generateHTML() feature(html): allow caller to specify a document implementation in generateHTML() Aug 18, 2023
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.

3 participants