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(pdf): Move annotation styling to box-annotations #1497

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

karelee7
Copy link
Contributor

@karelee7 karelee7 commented Aug 2, 2023

In v3.6.172 of PDFjs, a z-index styling was explicitly added to pdf_viewer.css: https://cdnjs.cloudflare.com/ajax/libs/pdf.js/3.6.172/pdf_viewer.css. This styling was not present in the previous version of PDFjs that we're using: https://cdnjs.cloudflare.com/ajax/libs/pdf.js/2.14.305/pdf_viewer.css and keeping this new styling causes issues with the other layers in preview (i.e. popup layer, highlighted layer, etc.). Updating styling for our layers resolves the issues that QA has discovered, but moving them to the box-annotations repo is preferable.

Have tested this change in the SDK with the stylings moved to box-annotations/linked with EUA.

.ba-Layer--popup,
.ba-Layer--highlight,
.ba-Layer--drawing,
.ba-point-annotation-marker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ba-point-annotation-marker class actually used anywhere? I didn't see this in box-annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also couldn't find it in annotations, but this needs to move above the text layer to be clickable (it's a fix for expiring embed links - since it's prefixed with ba I figured that annotations would be the best place to put it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's used in a legacy version of box-annotations that's only used by the Expiring Embed flow. So, unless we want to release a patch to v2 of box-annotations, this specific style will need to stay in box-content-preview.

@@ -9,10 +9,3 @@
.bp-AnnotationsControls-exitBtn {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

was ba-point-annotation-marker used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above answer!

@bfoxx1906 bfoxx1906 self-requested a review August 2, 2023 19:24
@karelee7 karelee7 merged commit 07fccf2 into box:master Aug 2, 2023
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.

3 participants