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

Make reading time field editable #270

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

vbh
Copy link
Contributor

@vbh vbh commented Jun 25, 2023

Currently reading time field is only readable, this PR adds a reading time field in the Add/Edit book screen which will be visible only if the status is finished. If reading time is not edited or set then the default logic of reading time (calculated based on start and finish dates) will be used.

@mateusz-bak
Copy link
Owner

mateusz-bak commented Jul 12, 2023

Could you fix the conflicts? It doesn't looks like a trivial change and didn't want to add it 2.1 but now I can take a look for 2.2.

@mateusz-bak
Copy link
Owner

Ok I reviewed it now, sorry it took long time.

  1. I think the custom reading time should be saved as milliseconds instead of the string.
  2. The custom time button should be displayed below start/end date when editing book.
  3. There's some unnecessary space when editing with in progress and unfinished book statuses.
  4. The statistics should take the new reading time into considerations - that's why i think the milliseconds field will work the best.

I'm doing some other stuff with the db etc and I'm happy to work on the changes I mentioned. Just let me know if you are okay or if you want to implement them yourself @vbh

Copy link
Owner

@mateusz-bak mateusz-bak left a comment

Choose a reason for hiding this comment

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

Looks good, just small mentioned changes

@mateusz-bak mateusz-bak merged commit 3101405 into mateusz-bak:master Nov 13, 2023
0 of 2 checks passed
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.

2 participants