Skip to content

Conversation

thamara
Copy link
Contributor

@thamara thamara commented Aug 28, 2019

Description

Word count on Note Details was incorrect computed by counting words splitted by space. In the event of one word per line, the count was incorrect:

wrong word count

This was fixed by converting line breaks into spaces and then splitting them for count:

correct word count

Issue fixed

#888

Type of changes

  • ⚫ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • ⚫ My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • ⚪ All existing tests have been passed
  • ⚫ I have attached a screenshot/video to visualize my change if possible

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 Congrats on your first PR being approved!

@ZeroX-DG ZeroX-DG added the approved 👍 Pull request has been approved by sufficient reviewers. label Aug 28, 2019
@@ -511,7 +511,7 @@ class MarkdownNoteDetail extends React.Component {
exportAsTxt={this.exportAsTxt}
exportAsHtml={this.exportAsHtml}
exportAsPdf={this.exportAsPdf}
wordCount={note.content.split(' ').length}
wordCount={note.content.replace(/\r?\n?\s+/g, ' ').trim().split(' ').length}

Choose a reason for hiding this comment

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

Hello ! :)

I think it can be replaced by:

note.content.trim().split(/\r?\n?\s+/g).length

or even

note.content.trim().split(/\s+/g).length
since \s includes cariage returns and new lines

https://www.w3schools.com/jsref/jsref_regexp_whitespace.asp

Copy link
Member

Choose a reason for hiding this comment

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

@thamara Can you change your code to the above suggestion for more concise code? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'd use the split(/s+/g).

https://repl.it/@alexanderwolf/Word-count-regex

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'll make the suggested change. Thanks!

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Sep 1, 2019
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Sep 1, 2019
@Rokt33r Rokt33r merged commit 2c103ac into BoostIO:master Sep 2, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Sep 2, 2019
@Rokt33r Rokt33r added this to the v0.13.0 milestone Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants