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

feat: add apple books highlight links #24

Conversation

absorpheus
Copy link
Contributor

@absorpheus absorpheus commented Jul 8, 2024

What is it?

This feature will fetch the EPUB Canonical Fragment Identifier for highlights so that they can be opened at the exact highlight location with Apple Books.

Summary

  • test(db): update migrations to enable new database tests

  • test: add basic highlight links tests for db

  • test: update mock tests, plugin docs and info

  • refactor(core): update default template to include highlight location

  • refactor(core): update types, constants, schemas, seedData and methods

  • fix: template for colored highlights. register handlebars custom helpers. Minor grammar fixes

  • fix: new lines no longer appear at the end of highlight blocks

  • docs(README): update preview and template-colors screenshots to show highlight links feature

  • docs(README): add highlight location template variable

  • docs(README): update template with highlight link

  • linter: update tsconfig to remove unnecessary type checks

* test(db): update migrations to enable new database tests

* test: add basic highlight links tests for db

* test: update mock tests, plugin docs and info

* refactor(core): update default template to include highlight location

* refactor(core): update types, constants, schemas, seedData and methods

* fix: template for colored highlights. register handlebars custom helpers. Minor grammar fixes

* docs(README): update preview and template-colors screenshots to show highlight links feature

* docs(README): add highlight location template variable

* docs(README): update template with highlight link

* linter: update tsconfig to remove unnecessary type checks
@absorpheus absorpheus changed the title Feature: Add Apple Books Highlight Links feat: Add Apple Books Highlight Links Jul 8, 2024
@absorpheus absorpheus changed the title feat: Add Apple Books Highlight Links feat: add apple books highlight links Jul 8, 2024
@bandantonio bandantonio self-requested a review July 8, 2024 16:16
Copy link
Owner

@bandantonio bandantonio left a comment

Choose a reason for hiding this comment

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

@absorpheus Thank you for the contribution, the PR looks great.
However, I see multiple updates that you're trying to deliver:

  • highlights link feature + tests and migrations
  • fix for Issue with Template Import #25 (I will take care of that one, it needs tests to be fixed as well)
  • fix for grammar

I would recommend you to split this PR into two, each one containing the corresponding changes. Please use this repository directly (not a fork) to make those changes, because the required checks weren't triggered for some reason for the PR. I will investigate why.
Thank you.

@@ -66,6 +66,7 @@ The plugin uses Handlebars and Markdown to customize the output of your highligh
- If you highlight parts of two adjacent sentences, the `contextualText` will contain both sentences.
- `{{{highlight}}}` - The highlighted text.
- `{{{note}}}` - A note you added for the highlight.
- `{{{highlightLocation}}}` - The EPUB CFI of the highlighted text. It is used to create a link to the highlighted text in Apple Books: `[Apple Books Highlight Link](ibooks://assetid/{{bookId}}#{{highlightLocation}})`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- `{{{highlightLocation}}}` - The EPUB CFI of the highlighted text. It is used to create a link to the highlighted text in Apple Books: `[Apple Books Highlight Link](ibooks://assetid/{{bookId}}#{{highlightLocation}})`.
- `{{{highlightLocation}}}` - a unique identifier of the highlighted text. It is used to create a link to the highlighted text in Apple Books. For example: `[Apple Books Highlight Link](ibooks://assetid/{{bookId}}#{{highlightLocation}})`.

@@ -132,6 +133,7 @@ Number of annotations:: {{annotations.length}}
{{#if (eq highlightStyle "4")}}- 🎯 Highlight:: <mark style="background:rgb(242,178,188); color:#000; padding:2px;">{{{highlight}}}</mark>{{/if}}
{{#if (eq highlightStyle "5")}}- 🎯 Highlight:: <mark style="background:rgb(214,192,238); color:#000; padding:2px;">{{{highlight}}}</mark>{{/if}}
- 📝 Note:: {{#if note}}{{{note}}}{{else}}N/A{{/if}}
- 📙 Highlight Link:: {{#if highlightLocation}}[Apple Books Highlight Link](ibooks://assetid/{{../bookId}}#{{highlightLocation}}){{else}}N/A{{/if}}
Copy link
Owner

@bandantonio bandantonio Jul 8, 2024

Choose a reason for hiding this comment

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

I believe it's not necessary to add a highlight link to this template, as its main focus is on colors.

Suggested change
- 📙 Highlight Link:: {{#if highlightLocation}}[Apple Books Highlight Link](ibooks://assetid/{{../bookId}}#{{highlightLocation}}){{else}}N/A{{/if}}

Copy link
Owner

Choose a reason for hiding this comment

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

I believe it's not necessary to add a highlight link to this template, as its main focus is on colors. So you can revert to the original screenshot.

@absorpheus
Copy link
Contributor Author

@bandantonio Thank you for your feedback on the PR. I will work on making these changes very soon.

@bandantonio
Copy link
Owner

@absorpheus No worries, I just released a new version to fix the bug with custom templates, in order to prevent user experience disruption. So there is no rush, take your time, please.

@absorpheus
Copy link
Contributor Author

@bandantonio Please give me permission to access the repository so I can push the feature branches and create the PR's. Thank you

@bandantonio bandantonio added invalid This doesn't seem right wontfix This will not be worked on and removed invalid This doesn't seem right labels Jul 11, 2024
@bandantonio bandantonio force-pushed the master branch 3 times, most recently from 5f627f2 to e5b071a Compare July 14, 2024 19:59
@bandantonio
Copy link
Owner

This PR was split into several ones:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants