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.

Sign up for GitHub

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

Jump to bottom

Word count: Adjust to count numbers as words #27288

Merged
merged 2 commits into from
May 14, 2021
Merged

Word count: Adjust to count numbers as words #27288

merged 2 commits into from
May 14, 2021

Conversation

phena109
Copy link
Contributor

@phena109 phena109 commented Nov 26, 2020 •
edited by gziolo
Loading

Description

Adjusted the character range to be removed so count logic can count numbers.
Fixes #26339

How has this been tested?

Used the unit test suite come with the repo. I have adjusted 1 existing case and added a new one

Types of changes

Adjusted the character range to be removed so count logic can count numbers. It doesn't break anything. All test related to the wordcount package passed

Checklist:

  • My code is tested.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 26, 2020
ambienthack
ambienthack approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@ambienthack ambienthack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on texts from similar issue (#17988) and PR looks good to me.
I don't think tests are failing because of your code. You can try to rebase to force tests to rerun.

@phena109
Copy link
Contributor Author

phena109 commented Dec 3, 2020 •
edited
Loading

Tested on texts from similar issue (#17988) and PR looks good to me.
I don't think tests are failing because of your code. You can try to rebase to force tests to rerun.

Thanks! But would you mind show me how to perform a rebase (or other ways to rerun the test)? I am really learning....

@ambienthack
Copy link
Contributor

  1. update you fork https://github.com/WordPress/gutenberg/blob/master/docs/contributors/git-workflow.md#keeping-your-fork-up-to-date
  2. rebase https://github.com/WordPress/gutenberg/blob/master/docs/contributors/git-workflow.md#keeping-your-branch-up-to-date

You'll still need a review from someone with write permissions (which i don't have) for PR to get merged. Good luck!

@skorasaurus skorasaurus added the [Package] Word count /packages/wordcount label Dec 10, 2020
Base automatically changed from master to trunk March 1, 2021 15:44
gziolo
gziolo reviewed May 14, 2021
View reviewed changes
packages/wordcount/src/test/index.test.js
characters_excluding_spaces: 15,
characters_including_spaces: 19,
},
{
message: 'Numbers as word',
string: 'Should be 4 words',
Copy link
Member

Choose a reason for hiding this comment

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

I did a quick cross-check with Pages from Apple and it produces the same result:

Screen Shot 2021-05-14 at 09 29 45

Copy link
Member

Choose a reason for hiding this comment

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

I see also a comment from @Soean in #26339 (comment):

Good point, I tested Microsoft Word, Google Docs and Libre office, all apps count numbers as a word. So I think we should change it in WordPress

gziolo
gziolo reviewed May 14, 2021
View reviewed changes
packages/wordcount/src/test/index.test.js
@@ -43,10 +43,17 @@ describe( 'WordCounter', () => {
{
message: 'Punctuation.',
string: 'It’s two three \u2026 4?',
words: 3,
words: 4,
Copy link
Member

Choose a reason for hiding this comment

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

The same sentence in Pages:
Screen Shot 2021-05-14 at 09 31 16

gziolo
gziolo reviewed May 14, 2021
View reviewed changes
packages/wordcount/src/defaultSettings.js
@@ -50,7 +50,7 @@ export const defaultSettings = {
'[',

// Basic Latin (extract)
'\u0021-\u0040\u005B-\u0060\u007B-\u007E',
'\u0021-\u002F\u003A-\u0040\u005B-\u0060\u007B-\u007E',
Copy link
Member

Choose a reason for hiding this comment

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

I confirm that it allows now digits from 0 to 9 👍🏻

gziolo
gziolo approved these changes May 14, 2021
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR needs a rebase or, a commit with a note in the CHANGELOG about the change applied.

@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label May 14, 2021
@gziolo gziolo changed the title Adjust to count numbers as words Wordcount: Adjust to count numbers as words May 14, 2021
@gziolo gziolo changed the title Wordcount: Adjust to count numbers as words Word count: Adjust to count numbers as words May 14, 2021
@gziolo gziolo merged commit 8331d12 into WordPress:trunk May 14, 2021
@github-actions
Copy link

Congratulations on your first merged pull request, @phena109! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 10.7 milestone May 14, 2021
@gziolo
Copy link
Member

gziolo commented May 14, 2021

@phena109, thank you for working on this PR and your patience. It took a while to land it 😅

I couldn't add changes to the branch so I will open a follow-up with a note about the changes applied to the @wordpress/wordcount package.

gziolo added a commit that referenced this pull request May 14, 2021
@gziolo
Word count: Add entry in the CHANGELOG file
61fd7b2 
See #27288.
@phena109 phena109 deleted the fix_wc branch August 24, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@ambienthack ambienthack ambienthack approved these changes

Assignees
No one assigned
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Word count /packages/wordcount [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 10.7
Development

Successfully merging this pull request may close these issues.

wordcount does not count numbers as a word
4 participants
@phena109 @ambienthack @gziolo @skorasaurus