Keep track of progress through books over time #354
Conversation
Commented out the new update type because it was breaking and I don't need it quite yet
Also move into their own section
|
wow this rules, I'm so stoked!! I'm going to take a closer look tomorrow but rad, thank you |
Doesn't work yet
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| <div class="progress-update is-small"> | |||
| on page | |||
| <input class="is-small" type="text" size="3"> | |||
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
It kinda depends - I'd say it's primarily for my personal tracking, but I might occasionally want to post an update. A feature that I haven't added yet is the ability to write a short note along with the progress update (e.g. "oh dang, this is getting good!"), and I'd be more likely to want to post those to my feed. |
|
ah yeah that makes a lot of sense! My first thought is that the simplest route would be to associate a progress update with a |
| <div class="field is-grouped is-small px-2"> | ||
| <div class="control">on page</div> | ||
| <div class="control"> | ||
| <input class="input is-small" type="text" size="3"> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
Wish the bot gave more information, but it looks like you're trying to make a div be an edit field. Using native HTML elements is better because, not only will it work across browsers effortlessly, but you get a dozen accessibility properties for free. |
Just use the base model's created_date
|
accesslintbot seems a little...overexcited here 😆 I don't see an issue tracker for it, I was thinking of just emailing their support with a link to this issue? |
Also add $@ to bw-dev migrations, and factor the shift 1 out
|
As for the PR itself, I just merged in the latest changes from main (love the reorganization!) and I think this is in an okay checkpoint to merge - there's still stuff I'd like to add, but I think it's reasonably complete and useful in its current state. I've rearranged the checklist accordingly. On the last item, I think we can punt on that for now, and just show the total if we have it, and not if we don't, and I think that's fine. Oh, I do want to go in and tidy up some of these migrations, there's no reason to have all these merge commits for a single PR, I'll collapse those down. |
cincodenada
force-pushed
the
progress_update
branch
from
72b9322
to
0af4863
Compare
|
Okay, actually I'll leave most of the migrations in place, cause I don't want to mess with them. There was just one I hadn't added yet (0037), and so I added that and in place of the 0036 merge, since that's a pretty simple substitution. |
| self.assertEqual(self.edition.readthrough_set.count(), 0) | ||
|
|
||
| self.client.post('/start-reading/{}'.format(self.edition.id), { | ||
| 'start_date': '2020-11-27', |
There was a problem hiding this comment.
this might have the side effect of creating a "started reading" status and then triggering a broadcast task, which could be why the tests are hanging. you can use with patch('bookwyrm.broadcast.broadcast_task.delay') if that's what's happening. Stopping celery and redis when you run the tests will show these errors locally. I have learned the hard way!
There was a problem hiding this comment.
Oh rad, that's very helpful, thanks! I'll take a peek at that, hopefully the logs will confirm once they time out again in a couple hours 🙃
|
I made a bunch of merge conflicts for you again! As before, I'd be happy to resolve them for you if that would be easier. |
|
Those merge conflicts were 🍰 😄, barely had to tell my diff tool anything, yay! I poked around at the |
|
oh patching for the whole class is great! I'm looking forward to testing tomorrow :) |
bookwyrm/templates/feed.html
Outdated
| @@ -47,7 +47,17 @@ <h2 class="title is-5">Your books</h2> | |||
| </div> | |||
| </div> | |||
| <div class="card-content"> | |||
| {% include 'snippets/shelve_button.html' with book=book %} | |||
| <div class="columns is-gapless"> | |||
There was a problem hiding this comment.
I think this works better just flat, rather than in columns -- so this markup minus all the added divs? it was too long for my screen as is
There was a problem hiding this comment.
I ended up flattening this and then totally reworking the update snippet. Curious to hear what you think, I'm...mostly happy with it I think 😅
| <div class="control"> | ||
| <input | ||
| aria-label="{% if readthrough.progress_mode == 'PG' %}Current page{% else %}Percent read{% endif %}" | ||
| class="input is-small" type="text" |
There was a problem hiding this comment.
this type should be number, rather than text for better input validation
bookwyrm/models/readthrough.py
Outdated
| class ProgressUpdate(BookWyrmModel): | ||
| ''' Store progress through a book in the database. ''' | ||
| user = models.ForeignKey('User', on_delete=models.PROTECT) | ||
| readthrough = models.ForeignKey('ReadThrough', on_delete=models.PROTECT) |
There was a problem hiding this comment.
When I try to delete a readthrough that has an associated progress update, I get an error because of the protected relationship. I think this is a case where CASCADE would be better than PROTECT?
There was a problem hiding this comment.
Good call, if a user is deleting a readthrough there's no reason to keep the progress updates. There's already a confirmation dialog for readthroughs, I bet I can add a "...and its X progress updates" to that just to make it clear.
bookwyrm/models/readthrough.py
Outdated
| ''' Store progress through a book in the database. ''' | ||
| user = models.ForeignKey('User', on_delete=models.PROTECT) | ||
| readthrough = models.ForeignKey('ReadThrough', on_delete=models.PROTECT) | ||
| progress = models.IntegerField() |
There was a problem hiding this comment.
I think this should have a min value validator to prevent negative values here
There was a problem hiding this comment.
I added validators here and on ReadThrough.progress (and tests to make sure they work), but didn't actually use them anywhere. I'm not sure they'll have any effect on ProgressUpdate, but might on the ReadThrough depending on how we use them?
In any case, having the validators is better than not
Add a warning about it, and update test to confirm it works
I don't think these validators will do anything unless we use them or are submitting a form, but they're there nonetheless
|
Whew, thank you so much for bearing with this, I'm stoked to have the feature. I think the next step is making it possible to post status update Generated Notes, and associated progress points with statuses |
|
I've been posting periodic updates on patreon with a list of newly added features. If you're okay with it, what's the best way to credit you for this? The default would just be like, First Name Last Name with a link to your website, but whatever feels best to you within reason is good for me |
|
Oh, sure, thanks! Joel Bradshaw is good, with a link to...uhh https://cincodenada.com I suppose. Or just my Mastodon account, whatever seems best! |
This works towards implementing #56, which is one of my personal must-haves for record-keeping purposes :)
This is very much an early draft that's very open for feedback, I'm not attached to any details of the implementation here.
What I've done so far:
pages_readfield and creates aProgressUpdaterecord on saveBase functionality:
Improvements:
Nice to have but not essential:
I think that's most of it, although I'll probably realize later I forgot something. Right now this is just on my local dev instance, but I'll probably put it up on my staging instance once I get it running again. Happy to hear any feedback on style/approach/etc!