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

Editor: Add word count #300

Merged
merged 3 commits into from
Nov 20, 2015
Merged

Editor: Add word count #300

merged 3 commits into from
Nov 20, 2015

Conversation

nylen
Copy link
Contributor

@nylen nylen commented Nov 20, 2015

This PR seeks to add a word count feature to the editor:

10 words

props @kellychoffman for the design. Closes #299.

Implementation notes

Right now the PostEditStore only sends a change event if the dirty or hasContent properties changed. So if the post is dirty, then further typing will not trigger changes. Word count should be closer to real-time though, hence the new rawContentChange event.

Test instructions

Verify that the word count appears correctly for new and existing posts and pages, and that it updates within 200ms after you stop typing (due to PostEditor#debouncedSaveRawContent).

Verify that the word count appears and updates correctly in both Visual and HTML modes.

Verify that the word count does not appear when the UI is in one of the following languages: Chinese (all variants), Japanese, Korean, Thai.

Still left to do

  • Improve CSS so that the word count is positioned correctly at all breakpoints.

For future follow-up

Chinese, Japanese, and Thai are character-based so they don't have a notion of word count. Here it would be more appropriate to count characters instead.

WP core has a different word count algorithm, including code to count characters. We should probably use this algorithm instead, because it will allow us to count characters, and Korean is at least one language where the TinyMCE algorithm won't work because its Unicode range is outside of what we're checking for.

Currently we hide the word count widget for these languages. Let's get the initial version out without CJK support and then circle back around to add this later.

@nylen nylen added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Nov 20, 2015
@kriskarkoski
Copy link
Contributor

This is testing well for me in Chrome 46/OS X 10.11

export default React.createClass( {
displayName: 'EditorWordCount',

mixins: [ React.addons.PureRenderMixin ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this component has no props, I don't think this is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I suppose it does make the component pure/render once, nm.

@timmyc
Copy link
Contributor

timmyc commented Nov 20, 2015

Played around with the branch and confirmed all of our discussion we had in slack around the new event being emitted. LGTM - so pending your css changes I think this is good to go.

if ( content && typeof content === 'string' ) {
content = content.replace( /\.\.\./g, ' ' ); // convert ellipses to spaces
content = content.replace( /<.[^<>]*?>/g, ' ' ); // remove HTML tags
content = content.replace( /&nbsp;|&#160;/gi, ' ' ); // remove space chars
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if make sense add spaces here especially if they aren't added in the below lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't quite sure what to do with that section. I suppose fa276d0 is fine too. Our coding guidelines seem to encourage things like this, though:

Use spaces liberally throughout your code. “When in doubt, space it out.”

and

Spaces may align code within documentation blocks or within a line

@MichaelArestad
Copy link
Contributor

After some discussion with @kellychoffman we decided to go with a non-fixed option for now. Exploring a sidebar location or a fixed version is something worth trying in the next iteration of it.

image

@nylen
Copy link
Contributor Author

nylen commented Nov 20, 2015

Hey, you fixed the weird padding thing too. Awesome, thanks for the CSS help 👍

@kellychoffman
Copy link
Member

👍 Looks good. Thanks again @MichaelArestad.

@kellychoffman kellychoffman added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 20, 2015
nylen added a commit that referenced this pull request Nov 20, 2015
@nylen nylen merged commit e85dcd6 into master Nov 20, 2015
@nylen nylen deleted the add/editor/word-count branch November 20, 2015 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor: Display word count for current post
7 participants