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: Show word count when text is selected in Editor #13530

Merged
merged 9 commits into from Jul 6, 2017

Conversation

@jeremeylduvall
Copy link
Member

jeremeylduvall commented May 1, 2017

This is an initial attempt at addressing #13488 and adding a word count when text is selected within the editor. Rather than adding an additional word count, which would get confusing, this replaces the existing word count for the entire post with the word count of the current selection when text is highlighted.

You'll notice I moved this.debouncedSaveRawContent into a separate call for this.onEditorKeyUp. That way, we can check the event to see if text was highlighted with the keyboard. I still need to check the keycodes to make sure they word across all platforms.

This code currently only works in the Visual editor as it doesn't appear as though we load everything in the HTML editor.

To Test

  1. Load this branch and visit the editor.
  2. Try selecting text. The word count should change based on your selection. Try clicking out of the text area and then back in (click buttons, title fields, switch editors, etc). The word count should adjust appropriately. Try using your keyboard to select text. Word count should work as intended.

GIF

wordcount

Addresses: #13488

@matticbot

This comment has been minimized.

Copy link

matticbot commented May 1, 2017

@mtias

This comment has been minimized.

Copy link
Member

mtias commented May 1, 2017

It'd be helpful to change the string to "x words selected" when it is counting from a selection.

@folletto

This comment has been minimized.

Copy link
Member

folletto commented May 2, 2017

It'd be helpful to change the string to "x words selected" when it is counting from a selection.

I came to say right this.

I'd also suggest an alternative option which I feel it would be better as it would preserve both counters:

screen shot 2017-05-02 at 10 42 35

@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented May 2, 2017

@folletto / @mtias what do you think about this new approach? The selected text is now using the darker $gray-text color:

screen shot 2017-05-02 at 6 48 33 am

Another option would be to mimic the same styling as Saved:

screen shot 2017-05-02 at 6 52 23 am

I prefer the latter approach.

(CircleCI is still failing. Since I modified createClass, I'll convert this to es6 so everything will be happy. Wanted to get the style/functionality approved first before doing that.)

@folletto

This comment has been minimized.

Copy link
Member

folletto commented May 2, 2017

I agree in mimicking saved. We might want the darker one, but in that case I'd change both... and I'll do that in a separate PR for clarity. ;)

@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented May 2, 2017

Updated to match the "Saved" text. Current appearance:

screen shot 2017-05-02 at 7 04 02 am

@jeremeylduvall jeremeylduvall force-pushed the fix/13488-selection-word-count branch from 17138f6 to 33efa16 May 4, 2017
@matticbot matticbot added [Size] L and removed [Size] M labels May 4, 2017
@jeremeylduvall jeremeylduvall force-pushed the fix/13488-selection-word-count branch from 33efa16 to f213c1d May 4, 2017
@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented May 4, 2017

Converted to ES6 and checks are now passing. I'm going to flip this to "Needs Review." Would appreciate some extra design eyes as well.

@folletto

This comment has been minimized.

Copy link
Member

folletto commented May 5, 2017

Design wise is 👍 for me.

I'd add just:

  1. Nitpick — can we color the / light gray instead of dark gray? Or just remove it entirely?
  2. Question — would it make sense to start showing it from 2+ words? Feels a bit odd that it tells me "1 word selected".

Awesome work. These kind of details are very useful :)

@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented May 5, 2017

Can we color the / light gray instead of dark gray?

Updated! Here's how it will look now:

screen shot 2017-05-05 at 7 10 01 am

Question — would it make sense to start showing it from 2+ words?

I have mixed feelings on this. It does feel odd to see "1 word selected." On the flip side, it feels weird to me to show the selected text count only at 2+ since 2 is a kind of arbitrary. The count is only really useful when it's difficult to manually count the words which might be 3, 4, 5, etc. I think it's a cleaner approach to just set a basic rule of "If text is selected, we show the count" instead of trying to determine a guideline of when the selected text would be useful.

@folletto

This comment has been minimized.

Copy link
Member

folletto commented May 5, 2017

Ok! Thanks :)

@jeremeylduvall jeremeylduvall force-pushed the fix/13488-selection-word-count branch from 6040467 to 53487ff May 16, 2017
@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented May 16, 2017

Added tests for the default behavior as well as the new selected text. They can be run using:

npm run test-client client/post-editor/editor-word-count

@mtias - if you have a moment this week, mind giving this another look?

CircleCI was failing due to this previous issue I believe. I've rebased. Should pass now. Still failing despite restarting a few times. Everything is passing locally. It looks like the error is unrelated to these changes.

@jeremeylduvall jeremeylduvall force-pushed the fix/13488-selection-word-count branch from 53487ff to ea38f15 May 16, 2017
@matticbot

This comment has been minimized.

Copy link

matticbot commented May 23, 2017

@jeremeylduvall This PR needs a rebase

@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented Jun 21, 2017

Thanks @aduth. I changed up the camelcase of TinyMCE and figured out why the tests weren't passing originally. Since we're now importing tinymce into the post editor, we have to mock the component within the tests. Tests should now be passing.

@matticbot

This comment has been minimized.

Copy link

matticbot commented Jun 30, 2017

@jeremeylduvall This PR needs a rebase

Currently, we don't update the word count shown in the editor when a
user highlights text. It would be nice if we updated the count to show
the word count of the selected text to match the functionality of other
editors.

Addresses: #13488
Instead of replacing the existing word count with the selected text,
this adds the selected text count in front of the word count when there
is text selected in the editor.
This updates the styling of the selected text indicator to match the
'Saved' text similarly placed at the bottom of the Editor.
This converts the EditorWordCount component to ES6 so the CircleCI tests
will pass.
This updates the logic used on keyup in the editor to grab the selected
text. We need to fire the getSelectedText function on any key up. This
ensures that the correct word count is shown when the words are
highlighted with a keyboard combination and also clears out the word
count if necessary on key up. This shouldn't be an issue because we
check whether or not state should be updated in getSelectedText.
This sets a span tag around the separator so we can set it to the light
gray color.
This commit adds tests for the EditorWordCount component to test both
the default and selected text behavior.
We're now importing TinyMce into the post editor component so we can use
the getContent() function. As a result, the tests for the post editor
were failing. This mocks the tinymce component in the editor so the
tests pass.
@jeremeylduvall jeremeylduvall force-pushed the fix/13488-selection-word-count branch from 800df14 to 3254c98 Jun 30, 2017
Copy link
Member

pento left a comment

This is looking really good, @jeremeylduvall!

I've just noted a few minor things. 🙂

@@ -213,6 +215,18 @@ export const PostEditor = React.createClass( {
}
},

getSelectedText: function() {

This comment has been minimized.

Copy link
@pento

pento Jul 1, 2017

Member

get*() method names traditionally refer to a method that returns a value. This would probably be better named copySelectedText(), storeSelectedText(), or something of that nature.

This comment has been minimized.

Copy link
@jeremeylduvall

jeremeylduvall Jul 4, 2017

Author Member

Makes total sense. I swapped it out for copySelectedText.

},

onEditorKeyUp: function() {
this.getSelectedText();

This comment has been minimized.

Copy link
@pento

pento Jul 1, 2017

Member

This should be a debounce()d call - recounting with every key up could have noticeable performance issues, particularly with large selections or on slower devices.

This comment has been minimized.

Copy link
@jeremeylduvall

jeremeylduvall Jul 4, 2017

Author Member

Great point - I added a 200ms debounce.

@@ -951,6 +969,10 @@ export const PostEditor = React.createClass( {

if ( mode === 'html' ) {
this.editor.setEditorContent( content );

if ( this.state.selectedText ) {
this.getSelectedText();

This comment has been minimized.

Copy link
@pento

pento Jul 1, 2017

Member

It took me a minute to figure out why getSelectedText() was being called when switching to HTML view. A comment here to note that it's being reset would be helpful.

@pento

This comment has been minimized.

Copy link
Member

pento commented Jul 1, 2017

Oh, one more thing - is there any particular reason for passing the selected text as a prop on EditorWordCount, rather than using the PostEditStore? It seems like it'd be clearer to use the same data flow methods for both the post content and the selected text.

To prevent performance issues on slower machines, we're debouncing the
selected text call on keyup by 200ms.
@jeremeylduvall

This comment has been minimized.

Copy link
Member Author

jeremeylduvall commented Jul 4, 2017

@pento thanks for the review! I addressed the minor items mentioned above. For the PostEditStore, I'm torn. If I understand correctly, the PostEditStore stores everything pertinent to a particular post in case we need to reference it elsewhere and to create one source of truth. Items specific to the session instance (sidebar and notice visibility, preview action, etc) are stored in editor state. In this case, I thought of the word count as something that was very fluid and part of the editor session, not specific to the post. Definitely open to the PostEditStore option if that's the way to go though.

@pento

This comment has been minimized.

Copy link
Member

pento commented Jul 6, 2017

That's a good point, it does seem to be part of the editor state, rather than a property of the post. Particularly as it's only available when we're in visual editor mode.

This PR looks good to me. 🙂

@pento
pento approved these changes Jul 6, 2017
@jeremeylduvall jeremeylduvall merged commit 40b6079 into master Jul 6, 2017
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
ci/i18n Total: 2 strings (1 new). 50% already translated.
Details
@jeremeylduvall jeremeylduvall deleted the fix/13488-selection-word-count branch Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.