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

Text and Underline annotation support #809

Merged
merged 17 commits into from
Mar 13, 2024

Conversation

michalrentka
Copy link
Contributor

@michalrentka michalrentka commented Nov 17, 2023

Closes #712

Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

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

Code looks good so far 👌


Interesting use of keyboard accessories, we could discuss if this UX is more appropriate for other annotation types as well. There is an issue of discoverability, I get those with double tap, is that correct?


FInally, a couple of times the text annotation drawing would zoom, without me changing anything about it. I only noticed after it happened, so I don’t have any hints as to what may have caused it. Dismissing and reopening the PDF, would restore what I expected.

@michalrentka
Copy link
Contributor Author

michalrentka commented Nov 21, 2023


Interesting use of keyboard accessories, we could discuss if this UX is more appropriate for other annotation types as well.

Other annotations don't really use keyboard as their main way of editing though. For text it makes sense.

There is an issue of discoverability, I get those with double tap, is that correct?

Yes. First tap is to select and the ability to move and scale text. Second tap is to confirm we actually want to edit text or its settings.


FInally, a couple of times the text annotation drawing would zoom, without me changing anything about it. I only noticed after it happened, so I don’t have any hints as to what may have caused it. Dismissing and reopening the PDF, would restore what I expected.

I'll try to reproduce, but it might be PSPDFKit issue.

@michalrentka michalrentka force-pushed the underlinetext branch 4 times, most recently from 4d1f14b to df5c4fe Compare November 28, 2023 09:23
@michalrentka michalrentka force-pushed the underlinetext branch 2 times, most recently from 321ac89 to 41a2f1a Compare November 30, 2023 09:55
@michalrentka michalrentka force-pushed the underlinetext branch 2 times, most recently from 971d033 to 90a360c Compare February 20, 2024 13:53
@michalrentka michalrentka force-pushed the underlinetext branch 2 times, most recently from 87142d7 to 7e7a918 Compare March 5, 2024 14:28
@michalrentka
Copy link
Contributor Author

@mvasilak can you take another look at this? I updated and fixes some things, we'll be merging this with free text and underline annotations disabled in UI for the time being.

Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

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

AnnotationViewController.swift is not part of the project, is it still in use?

Icons for the new types are still missing, right?

@michalrentka
Copy link
Contributor Author

Icons for the new types are still missing, right?

Yes, I created a ticket for it #871. They won't be visible for now though.

@michalrentka michalrentka merged commit 0802b12 into zotero:master Mar 13, 2024
1 check failed
@michalrentka michalrentka deleted the underlinetext branch March 13, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support underline and text annotations
2 participants