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

A brand new CharacterCount extension #2256

Merged
merged 9 commits into from
Dec 8, 2021

Conversation

philippkuehn
Copy link
Contributor

@philippkuehn philippkuehn commented Dec 8, 2021

With this PR we introduce the redesigned CharacterCount extension. It’s a full re-write and fixes some bugs.

New features

Add mode option

You can now choose between textSize and nodeSize for calculating the character size. Defaults to textSize.

CharacterCount.configure({
  mode: 'nodeSize',
})

Add characters() storage method

Get the number of characters for the current document with this new storage method. This deprecates editor.getCharacterCount().

editor.storage.characterCount.characters()

// Get the size of a specific node.
editor.storage.characterCount.characters({ node: someCustomNode })

// Overwrite the default `mode`.
editor.storage.characterCount.characters({ mode: 'nodeSize' })

Add words() storage method

Get the number of words for the current document with this new storage method.

editor.storage.characterCount.words()

// Get the number of words for a specific node.
editor.storage.characterCount.words({ node: someCustomNode })

Preview

https://deploy-preview-2256--tiptap-embed.netlify.app/preview/Extensions/CharacterCount

Fixes #1049
Fixes #1550
Fixes #1839
Fixes #2245

@netlify
Copy link

netlify bot commented Dec 8, 2021

✔️ Deploy Preview for tiptap-embed ready!

🔨 Explore the source changes: 04ca755

🔍 Inspect the deploy log: https://app.netlify.com/sites/tiptap-embed/deploys/61b0f23062da120007b4bac3

😎 Browse the preview: https://deploy-preview-2256--tiptap-embed.netlify.app

@hanspagel
Copy link
Contributor

So cool! Thanks for putting so much effort into it!

@philippkuehn philippkuehn merged commit 5daa870 into main Dec 8, 2021
@philippkuehn philippkuehn deleted the feature/character-count-improvements branch December 8, 2021 20:26
@caroillemann
Copy link

caroillemann commented Dec 9, 2021

Hi :) I just tested your deploy-preview url and it seems that the first word on a new line isn't added to the total number of words. As you can see here the word 'And' is word number 4 and 'more' is number 5 - but the total count is only 4.

Screenshot 2021-12-09 at 12 55 17

@philippkuehn
Copy link
Contributor Author

@caroillemann Oh you are right. Should be fixed with this patch.

@rfgamaral
Copy link
Contributor

rfgamaral commented Dec 10, 2021

@philippkuehn Just noticed editor.getCharacterCount was deprecated, and arrived here after trying to implement the new options. I'm using this with TypeScript, and I'm not sure about some decisions, here are my suggestions:

  1. Although the docs have editor.storage.characterCount.characters() as an example, the characters function is expecting a required options object:
    characters?: (options: {
    node?: ProseMirrorNode,
    mode?: 'textSize' | 'nodeSize',
    }) => number,

    I think options should be made optional so that you can actually call characters() without arguments.
  2. I'm wondering if we can avoid having characters and words being optional by moving the functions from onBeforeCreate to addStorage instead (that way they wouldn't be undefined)

With both of these changes, we couldn't need to call characters?.({}) || 0 like this, it would be much better if we could just call .characters().

To finish, I have a question about this issue:

image

The only I can think of to work around this is:

const count = (editor.storage.characterCount as CharacterCountStorage).characters?.({}) || 0

And I'm wondering if there's a better way without casting?

@philippkuehn
Copy link
Contributor Author

Oh, the first bug should be fixed with this patch.

We can’t move these methods to addStorage because there is no editor instance at this moment. That’s why it’s technically correct that these methods are optional. But I agree that it can be a bit confusing. I already thought about making the storage type required, even if it wouldn't be technically correct. Probably the DX would be a bit better anyway.

@rfgamaral
Copy link
Contributor

We can’t move these methods to addStorage because there is no editor instance at this moment.

Granted that you have a lot more context than me, what if you check for this.editor first, and if it's not available, return 0? Wouldn't that work? 🤔

@philippkuehn
Copy link
Contributor Author

@rfgamaral There is no this.editor context within addStorage at all so this won’t work.

@rfgamaral
Copy link
Contributor

@philippkuehn Got it! What about having dummy functions that just return 0 instead of being set as undefined? Once onBeforeCreate is called, these dummy functions would be replaced.

@philippkuehn
Copy link
Contributor Author

@rfgamaral yeah, I like that 👍 Fixed.

@rfgamaral
Copy link
Contributor

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants