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

Tweak getTextBetween to prepend blockSeparator to block node even if block node doesn't have any children #5055

Merged
merged 2 commits into from
May 10, 2024

Conversation

hivokas
Copy link
Contributor

@hivokas hivokas commented Apr 11, 2024

Please describe your changes

Currently getTextBetween function doesn't prepend blockSeparator to block nodes without children.

This PR updates it to prepend blockSeparator to block nodes in any case (even if block node doesn't have any children).

How did you accomplish your changes

I have tweaked the implementation to not depend on child nodes of blocks when deciding whether blockSeparator should be prepended.

How have you tested your changes

I used preview link generated by Netlify bot below.

How can we verify your changes

Use preview link generated by Netlify bot below.

Remarks

Below are before/after examples.

Without empty paragraph

Hello!
How are you?

Before: 'Hello!'+BLOCK_SEPARATOR+'How are you?'
After: 'Hello!'+BLOCK_SEPARATOR+'How are you?'

With empty paragraphs

Hello!

How are you?

Before: 'Hello!'+BLOCK_SEPARATOR+'How are you?'
After: 'Hello!'+BLOCK_SEPARATOR+BLOCK_SEPARATOR+'How are you?' (it's breaking change, but I consider it a bugfix)

Checklist

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

Related issues

N/A

Copy link

netlify bot commented Apr 11, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 7cfb56f
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/662f3ad4b2dfa50008d74d88
😎 Deploy Preview https://deploy-preview-5055--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.

@hivokas hivokas marked this pull request as ready for review April 11, 2024 09:38
@hivokas hivokas changed the title Fix getTextBetween Tweak getTextBetween to prepend blockSeparator to block node even if block node doesn't have any children Apr 11, 2024
@hivokas hivokas changed the title Tweak getTextBetween to prepend blockSeparator to block node even if block node doesn't have any children Tweak getTextBetween to prepend blockSeparator to block node even if block node doesn't have any children Apr 11, 2024
@knitevision1
Copy link

knitevision1 commented Apr 15, 2024

@bdbch @svenadlung +1 on this one
Currently concurrent empty like breaks are not reflected in getText()

E.g. in the screenshots between line 2 and 5 there are 2 paragraphs (line breaks), but getText only returns one \n

getText vs DOM:

Selection_414

Selection_413

@hivokas
Copy link
Contributor Author

hivokas commented Apr 18, 2024

@knitevision1 if this PR gets merged, you (and I) will be able to achieve the desired result with getText({ blockSeparator: '\n' }) (default separator is \n\n).

@hivokas
Copy link
Contributor Author

hivokas commented Apr 29, 2024

@bdbch could you take a look at this PR when you get a moment? Looking at reactions in this PR, it looks like I'm not the only one who wants this change.

@bdbch bdbch merged commit edceec4 into ueberdosis:main May 10, 2024
14 checks passed
@hivokas
Copy link
Contributor Author

hivokas commented May 10, 2024

Thanks @bdbch!

@itzcull
Copy link

itzcull commented May 14, 2024

When will this get released? So happy with this change, it's made dealing with whitespaced text content so annoying

@hivokas
Copy link
Contributor Author

hivokas commented May 15, 2024

@itzcull it's released 🎉

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

4 participants