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: #4327 #4444

Closed
wants to merge 1 commit into from
Closed

fix: #4327 #4444

wants to merge 1 commit into from

Conversation

tonyhallett
Copy link
Contributor

Please describe your changes

Fix issue 4327

It uses Node.textContent which uses NodeSpec.leafText to fill it. But Tiptap does not fill NodeSpec.leafText using Tiptap's Node.renderText, rather Tiptap has its own separate mechanism to generate text content from Nodes

How did you accomplish your changes

Used Tiptap method of getting text, which use renderText, instead of textContent which does not.

How have you tested your changes

Added a new example with associated spec.

How can we verify your changes

Run the test or view the example and see that there is no menu shown editor has focus.

Remarks

Note that the spec is currently not with the ui example due to cypress issue on windows. (Perhaps demos folder could go in tests ? )
If pr is approved then I will move the spec in to the demos folder.
Also the ui is in Examples - do you want in Experiments / a new Issues folder ?
Should I expose a helper - getText(node,schema,blockseparator) ?

As this change uses TipTap renderText and not ProseMirror leafText the result will differ if Node extension is provided that has added leafText with extendNodeSchema or changed the schema in onBeforeCreate.
I don't think that any public repos are doing this.
https://github.com/search?q=tiptap+leafText&type=code
Regardless, The getTextSerializersFromSchema method could be changed to invoke the spec leafText if it had been provided. This would also ensure that editor.getText, generateText and the clipboardTextSerializer can also work with leafText.

Similar issues to this one where renderText is not used ( and spec.leafText is )
Node.textContent is used in other places in Tiptap. Potentially issues there too.
Node.textBetween => Fragment.textBetween => invokes leafText and not renderText.
Should Tiptap usages of Node.textBetween be changed to use Tiptap getTextBetween ( and schema text serializers) ?

renderText and how to ensure it is used is not well documented in the docs. This is the only mention
https://tiptap.dev/api/schema

Here is the example from the Mention extension:

renderText({ node }) {
  return `@${node.attrs.id}`
},

(This is not the case anymore for mention extension )

  renderText({ node }) {
    return this.options.renderLabel({
      options: this.options,
      node,
    })
  },

As such users expect renderText to be used when using the ProseMirror api - #4310

It is possible to add leafText to the spec so that it calls renderText although the signatures are different. ( Could use onBeforeCreate to get the editor and the doc for parent and index but impossible to get the pos argument. )

Checklist

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

Related issues

#4327

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit baf6947
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/65018a2468f3b600081a9590
😎 Deploy Preview https://deploy-preview-4444--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 nperez0111 force-pushed the develop branch 2 times, most recently from bcaef5c to 7e7ae19 Compare June 12, 2024 04:33
@bdbch bdbch deleted the branch ueberdosis:develop June 26, 2024 21:57
@bdbch bdbch closed this Jun 26, 2024
@bdbch bdbch reopened this Jun 27, 2024
@bdbch
Copy link
Member

bdbch commented Nov 30, 2024

@tonyhallett I moved your commit over to #5838 and added a little bit on top. Closing this PR for that.

@bdbch bdbch closed this Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants