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

Add Word Count #1624

Conversation

ryanwelcher
Copy link
Contributor

Adding a the Word Count feature as mention in #1557.

There is an issue with load order due to #1569 in that the WordCounter is not available when the component is initialized. Ideally, the core wp.utils should be loaded before Gutenbergs

@ellatrix
Copy link
Member

ellatrix commented Jul 1, 2017

Some thoughts:

  • You'll need to create an instance, not use the prototype directly. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.
  • It will definitely need to be debounced. See https://lodash.com/docs/#debounce. In core it's 1 second.
  • I'm not sure if we'll want to serialise the content. Maybe we need something in the block registration that returns the attributes which need to be used as part of the global word/character count. If we do serialise, this should be in the debounced function.
  • I wonder if something like "# words" would be nicer than just "Word Count: #".

For an example in core, see https://github.com/WordPress/WordPress/blob/4.8/wp-admin/js/post.js#L1216.

@nylen
Copy link
Member

nylen commented Jul 1, 2017

Replying to #1569 (comment) here:

I am running into an issue where WordCounter is not available when the component initializes as it is loaded after Gutenberg. I have accommodated for that in the component but it would be better if the core items were loaded first. However, this puts us back to the issue of the namespace being overridden when Gutenberg loads.

This is fixed in 8b1a7bf and the stopRender property is removed.

The relevant portion of the script dependency tree now looks like this:

wp-editor
    - depends on wp-utils
    - depends on word-count
        - depends on wp-utils

I think this is a good exercise for playing nice with the rest of core's JS and we should leave it in place at wp.utils.

@nylen
Copy link
Member

nylen commented Jul 1, 2017

Echoing @iseulde's feedback above and adding some further notes.

I am not sure that "Post Settings" is the best place for this. You can't see the word count while writing, because it lives inside the base post settings, but when a block is selected, the block's settings appear in the inspector instead. However, while writing is the time that you most want to see the word count. We should probably put this in a more prominent location, cc @jasmussen @mtias for design thoughts.

We will need to be a good bit more clever with serializing the post content, as this can be a very expensive operation, and currently it is being done for every single Redux action. One possibility would be to simply debounce the serialization. Another would be to make the Editable component count words itself, store these counts in Redux state, then add them together to generate the final count.

While there are a lot of code differences (Redux vs Flux, for one), it's probably worth taking a look at the PR that introduced this in Calypso:
Automattic/wp-calypso#300. The corresponding operation is performed every 200ms; here is the line responsible for the debouncing.

We had to add a new event for this - rawContentChange - because the editor content is not updated during regular typing. The same is true for Gutenberg: we do not send any Redux actions while a user is typing within a block; however, the word count really should be updating during this time as well.

Finally this PR should probably have a more human-readabale title, such as simply "Add word count feature".

@ellatrix ellatrix changed the title Feature/post settings word count Add word count Jul 1, 2017
@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Component] TinyMCE [Type] Enhancement A suggestion for improvement. labels Jul 1, 2017
@ellatrix
Copy link
Member

ellatrix commented Jul 1, 2017

The same is true for Gutenberg: we do not send any Redux actions while a user is typing within a block; however, the word count really should be updating during this time as well.

Right, currently Editables are not synced on typing, only blur, while inputs and textareas are. This will probably change in the future, also to enable us to manage undo levels.

@jasmussen
Copy link
Contributor

I am not sure that "Post Settings" is the best place for this. You can't see the word count while writing, because it lives inside the base post settings, but when a block is selected, the block's settings appear in the inspector instead. However, while writing is the time that you most want to see the word count. We should probably put this in a more prominent location, cc @jasmussen @mtias for design thoughts.

James makes an excellent point here, however I think this is a question that might be worth opening up wider, because in my personal opinion, the word count feature isn't that useful. And because of this, I like the idea behind where @jwold put it in the mockup in #1557 — it's in Post Settings which means if you need to see it, it lives where the document metadata is. But it is not present when the post settings sidebar is closed, which is the closest we have to a "distraction free" environment.

However personal opinions should probably not inform where this lives. If a bunch of people rely on this to the point where they want the word count to be accessible even with the sidebar toggled off, we can utilize one of the bottom corners of the editor for a floating counter, like we do on WordPress.com:

screen shot 2017-07-03 at 10 25 15

However this is not a great UI either, as it brings a bunch of headaches to mobile and small viewports. When you've written a long document and your caret is at the end of the doc, it gets very crowded with a word counter that's near where you're writing. Which is another argument for putting it in post settings: there when you need it, not there all the time.

@jwold
Copy link

jwold commented Jul 3, 2017

In my experience keeping this sort of info (word count, paragraph count, character count, read time) makes sense to keep hidden with one or two clicks. I'd vote to keep it in post settings unless there's a strong user case for pulling it out.

@ryanwelcher ryanwelcher changed the title Add word count Introduce Word Count component. Jul 4, 2017
@ryanwelcher ryanwelcher changed the title Introduce Word Count component. Add Word Count Jul 4, 2017
@nylen
Copy link
Member

nylen commented Jul 5, 2017

Ok, given the above discussion, for a first iteration it seems fine to me to solve some of the efficiency issues via a debounce and leave word count in the sidebar. However, it's not really clear to me where this debounce code should go.

/**
* WordPress dependencies
*/
import { __ } from 'i18n';
Copy link
Member

Choose a reason for hiding this comment

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

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master

@gziolo
Copy link
Member

gziolo commented Sep 22, 2017

It was superceded by #2684. Should we close this one?

@gziolo
Copy link
Member

gziolo commented Sep 22, 2017

After some thinking, I decided to close this one. Thank your for investing your time on this PR. Much appreciated. Feel free to reopen this one if you think it is still relevant.

@gziolo gziolo closed this Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants