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

Added image counting to the {{reading_time}} helper #9366

Merged
merged 4 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@rkalis
Contributor

rkalis commented Jan 3, 2018

issue #9200

  • Added imageCount utility, which counts images using an img-tag regex, amended from the general tag-regex found in wordCount
  • Added this imageCount to the {{reading_time}} helper, adding 12 seconds to the reading time for every image
  • The feature image is still counted as before
Added image counting to {{reading_time}} helper
issue #9200
- Added imageCount utility, which counts images using an img-tag regex, amended from the general tag-regex found in wordCount
- Added this imageCount to the {{reading_time}} helper, adding 12 seconds to the reading time for every image
- The feature image is still counted as before
@ErisDS

This comment has been minimized.

Member

ErisDS commented Jan 3, 2018

Hi @rkalis! Thanks for taking the time to implement this.

I wanted to flag up that the article referenced in #9200 recommends an algorithm where it adds 12 seconds for the first image, 11 for the second, 10 for the 3rd and so on.

Would you mind updating your code to follow this pattern?

Thanks again!

Changed the image counting algorithm, as per https://blog.medium.com/…
…read-time-and-you-bc2048ab620c

issue #9200
- The first image adds 12 seconds, the second 11, the third 10, and so on
- Images from the tenth onwards add 3 seconds to the reading time
- I thought about using a consecutive numbers sum algorithm to avoid writing a for loop, but I think this for loop keeps the code a lot more readable

@rkalis rkalis force-pushed the rkalis:improved-reading-time branch from 3ce7905 to b4f6e9e Jan 3, 2018

@rkalis

This comment has been minimized.

Contributor

rkalis commented Jan 4, 2018

I have a different commit ready for adding a reading speed (wpm) option to the helper, would you like it in the same PR, or should I open a new one (is it even still a wanted feature)?

@rkalis rkalis force-pushed the rkalis:improved-reading-time branch from e3c764b to 9c01205 Feb 1, 2018

@rkalis rkalis changed the title from Added image counting to {{reading_time}} helper to Added image counting and a wpm option to the {{reading_time}} helper Feb 1, 2018

@rkalis rkalis force-pushed the rkalis:improved-reading-time branch from 9c01205 to e366f2d Feb 1, 2018

@rkalis

This comment has been minimized.

Contributor

rkalis commented Feb 1, 2018

I added the commit for the wpm option, cleaned up the tests as well. Also made sure it has no conflicts with the changes made with the translatability changes.

Added wpm option to {{reading_time}} helper
issue #9200
- Added a wpm option to the {{reading_time}} helper
- Fixed word count of one test
- Added a variable to the tests file which represents the article (250 words), since this can be used (with additions) by every test

@rkalis rkalis force-pushed the rkalis:improved-reading-time branch from e366f2d to 7ca7af4 Feb 1, 2018

@kirrg001

This looks good to me from the code perspective. Have not tested it.

@ErisDS

This comment has been minimized.

Member

ErisDS commented Feb 16, 2018

Really sorry for dropping the ball on this. Code looks good 👍 However, I would prefer not to include the wpm option for now.

It was listed on the original issue as a potential future improvement, but it's the kind of thing I would only add if it was often-requested. So if it had been requested a lot, we would definitely add it, but so far I've not heard a single request for it.

Every option adds maintenance & documentation overhead, and therefore it's best not to add anything unless we're certain it will be used.

Other than that this is good to go IMO.

Rolled back wpm feature for {{reading_time}} helper
issue #9200
- Removed the wpm feature from the reading time helper
- Still kept the options initialisation in there, since it's also added in the i18n updates
- Kept the tidied up tests as well
@rkalis

This comment has been minimized.

Contributor

rkalis commented Feb 16, 2018

No worries, I understand it's a big project, there's a lot that needs attention :).

Sounds logical to try to keep the amount of features limited to those that are actually being used. It won't be a lot of effort either to add it back in if it would be requested more often in the future 👍 .

I pushed a rollback for the wpm feature, while keeping the changes made to the tests.

@rkalis rkalis changed the title from Added image counting and a wpm option to the {{reading_time}} helper to Added image counting to the {{reading_time}} helper Feb 16, 2018

@ErisDS

This comment has been minimized.

Member

ErisDS commented Feb 16, 2018

Awesome, thanks for the understanding and for the help getting this implemented. I'll let @kirrg001 do the merge, releases are every Tuesday 😄

@kirrg001 kirrg001 merged commit 301e1b2 into TryGhost:master Mar 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rkalis

This comment has been minimized.

Contributor

rkalis commented Mar 5, 2018

When your pull request to TryGhost/Ghost gets merged

Mr Open Source

On to the next 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment