Convert the page view to ES6 syntax #8455
Conversation
web/pdf_page_view.js
Outdated
| }, | ||
| update(scale, rotation) { | ||
| this.scale = scale || this.scale; | ||
| this.rotation = rotation || this.rotation; |
There was a problem hiding this comment.
This isn't correct, since it will effectively prevent setting this.rotation to zero, if the rotation has previously been set to a non-zero value; please revert this change!
There was a problem hiding this comment.
Good point! I'm not entirely sure how I missed that, but I reverted the change and added a small comment to make it more clear that this structure is intended.
| this.id = id; | ||
| this.renderingId = 'page' + id; | ||
| this.pageLabel = null; | ||
| constructor(options) { |
There was a problem hiding this comment.
A more general question here: Should the signature be changed to constructor({ ... }) { instead, or would that be overly verbose considering the amount of parameters involved?
There was a problem hiding this comment.
I chose to not do that because it would indeed be too verbose. I usually keep a limit at five or six parameters: if we have more, than such an expansion does not provide much more insight in my opinion. Another example is the toolbar, which also has a huge amount of parameters.
timvandermeij
force-pushed
the
es6-page-view
branch
from
3b2844a to
0a3acb6
Compare
|
/botio-linux preview |
| * @param {PDFPageViewOptions} options | ||
| */ | ||
| function PDFPageView(options) { | ||
| var container = options.container; |
There was a problem hiding this comment.
I think it'd be a little bit clearer if we keep let container = options.container; here, considering that otherwise you're handling all the other parameters except this one at the top of the constructor.
There was a problem hiding this comment.
Done in the new commit.
| var container = options.container; | ||
| var id = options.id; | ||
| var scale = options.scale; | ||
| var defaultViewport = options.defaultViewport; |
There was a problem hiding this comment.
Similarily, let's keep let defaultViewport = options.defaultViewport; such that we can maintain the slightly simpler this.pdfPageRotate = defaultViewport.rotation; below.
There was a problem hiding this comment.
Done in the new commit.
web/pdf_page_view.js
Outdated
|
|
||
| let viewport = this.viewport; | ||
| let canvas = document.createElement('canvas'); | ||
| canvas.id = 'page' + this.id; |
There was a problem hiding this comment.
Nit: Pre-existing issue, which I've been annoyed with for a while but haven't bothered to fix: Can we do canvas.id = this.renderingId; instead.
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
es6-page-view
branch
from
0a3acb6 to
6dfdff2
Compare
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/868720482960d83/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/868720482960d83/output.txt Total script time: 2.04 mins Published |
|
/botio lint |
From: Bot.io (Linux m4)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/037c796a2cd8fd8/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/cb0c4de19a759f3/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/037c796a2cd8fd8/output.txt Total script time: 0.99 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/cb0c4de19a759f3/output.txt Total script time: 3.54 mins
|
Convert the page view to ES6 syntax
Easier reviewing with https://github.com/mozilla/pdf.js/pull/8455/files?w=1.