Add text bookmarks #439
Add text bookmarks #439
Conversation
Before, the book_id and page number were were being used to find the appropriate Text when editing and deleting bookmarks. Using the Text ID directly simplifies these queries. Note: With the current page slider, it is inconvenient to access and update the current Text.TxID of the current page. So the "Add bookmark" action from the slideout still uses the `book_id` and `page_num` to access the appropriate Text record when adding a new bookmark.
Fix the Text <-> TextBookmark relationship and make deletes cascading
|
Thank you very much @cblanken ! I hope to start review today. |
There was a problem hiding this comment.
This looks super, @cblanken . My main request is that bookmark edits and deletes should use the bookmark ID to find the database entity. (Some app designers prefer to use a "business key" rather than expose real entity IDs, but IMO we can stick w/ the IDs.)
If you're too busy to make these changes, LMK. This PR is very clear (👍🎉😄) and I can make the changes when I test it out locally. Super stuff!
|
Thanks for the review. I should have some time to fixup everything tomorrow 👍 |
|
Oops. So it begins...me forgetting to run the lint before commiting now that the lint hook is disabled by default 😞 |
|
Looks good! There are a few very small things (e.g. the bookmark title isn't required for a deletion, etc), but I'll fix them up when I check out this branch locally to try it out. I'll just commit those changes to my local branch, you'll see them when it's merged. I'll let you know if I get stuck, thanks very much @cblanken 👍 |
|
Launched in 3.5.0. 🎉 |
This PR addresses issue #86 to add text bookmarks.
@jzohrab I'm pretty sure I've hit all the main requirements for the backend and smoke tests. Let me know if there's anything else I need to add or fixup before it would be ready to merge.
I think my next steps will probably be to add some demo data and maybe a mention of bookmarks in the Tutorial / Tutorial follow-up chapter and some basic acceptance tests.
I just wanted to get this PR posted before I lost steam :)