-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Annotations - _preparePopup method replaced with MarkupAnnotation #10728
Annotations - _preparePopup method replaced with MarkupAnnotation #10728
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge with these comments addressed and passing tests.
src/core/annotation.js
Outdated
@@ -598,6 +580,26 @@ class AnnotationBorderStyle { | |||
} | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment block can be removed because it only states where to find it in the spec, which is something we never really do elsewhere unless really required to understand the code, which isn't the case here.
src/core/annotation.js
Outdated
class MarkupAnnotation extends Annotation { | ||
constructor(parameters) { | ||
super(parameters); | ||
let dict = parameters.dict; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let
should be const
.
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/46aafe60ec2709d/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b3d81eda1da227e/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b3d81eda1da227e/output.txt Total script time: 17.78 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/46aafe60ec2709d/output.txt Total script time: 25.30 mins
Image differences available at: http://54.215.176.217:8877/46aafe60ec2709d/reftest-analyzer.html#web=eq.log |
Comments addressed, but I have no idea what to do with that failed test ... there aren't any annotations ... so no idea what is wrong with that ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…e class. This is just refactoring, so it shouldn't break anything. It should move annotation API closer to PDF spec and enable future expansion.
Oh ... ok ... sorry ... |
70f40a8
to
d96267c
Compare
About the test … there is a color change of column separator in table … reference says #5c5c5c and test says #5b5b5b .. I really have no clue how or why this changed … did something like this happen before? Is it a bad idea to run that test again ? Edit: Ok, I checked other PRs and it seems like a thing, that just happens from time to time … tricky :) |
Thank you! (The reference test failure is intermittent and unfortunately happens from time to time. It's not related to your patch.) |
_preparePopup method replaced with MarkupAnnotation base class.
This is just refactoring, so it shouldn't break anything. It should move annotation API closer to PDF spec and enable future expansion.