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

Setting for reading time #1767

Merged
merged 7 commits into from
Mar 18, 2016
Merged

Setting for reading time #1767

merged 7 commits into from
Mar 18, 2016

Conversation

nicosomb
Copy link
Member

@nicosomb nicosomb commented Mar 8, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documentation yes
Translation yes
Fixed tickets N/A
License MIT

Add a setting in config screen to define the reading speed for each user (I read so fast, I read so slowly, etc.)
TODO :

  • tests
  • translations
  • documentation
  • baggy view
  • add label (instead of the numeric value on config screen)

@tcitworld
Copy link
Member

Relaunched the Travis task that failed to clear cache for some reason.

@@ -29,6 +29,9 @@ public function getConfigTreeBuilder()
->integerNode('rss_limit')
->defaultValue(50)
->end()
->integerNode('reading_speed')
->defaultValue(200)
Copy link
Member

Choose a reason for hiding this comment

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

Why 200 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix. I didn't fix some tests.

@nicosomb nicosomb force-pushed the v2-setting-for-reading-time branch from bd0483f to 91bcc92 Compare March 10, 2016 12:20
@nicosomb nicosomb force-pushed the v2-setting-for-reading-time branch from 91bcc92 to 6432b92 Compare March 14, 2016 12:49
@nicosomb
Copy link
Member Author

I removed the slider to put a list. More readable.
I also added this text, below the list:

You can use online tools to estimate your reading speed (like this one).

capture du 2016-03-16 12 53 51

Reading speed
~~~~~~~~~~~~~

wallabag calculates a reading time for each article. You can define here, thanks to the slider, if you are
Copy link
Member

Choose a reason for hiding this comment

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

This is not a slider anymore

@nicosomb
Copy link
Member Author

Just improved tests ...
I don't know if we have to add a test for this line https://github.com/wallabag/wallabag/pull/1767/files#diff-7422bc94ce6c3b3e5fd993868d036c88R53

@j0k3r
Copy link
Member

j0k3r commented Mar 18, 2016

I'm ok with that PR.
We just need to remember that if one day we are chaging the 200 values from Tools/Utils.php every not default reading speed will be wrong.

@@ -248,6 +255,30 @@ public function getRssLimit()
}

/**
* Set readingSpeed.
*
* @param int $readingSpeed
Copy link
Member

Choose a reason for hiding this comment

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

This should be float right? (same for the getReadingSpeed)

@nicosomb nicosomb force-pushed the v2-setting-for-reading-time branch from ee57dcf to 1b64a84 Compare March 18, 2016 13:01
j0k3r added a commit that referenced this pull request Mar 18, 2016
@j0k3r j0k3r merged commit f39e94a into v2 Mar 18, 2016
@j0k3r j0k3r deleted the v2-setting-for-reading-time branch March 18, 2016 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants