Convert the hand tool to a class #7207
Conversation
|
|
||
| if (this.toggleHandTool) { | ||
| this.toggleHandTool.addEventListener('click', this.toggle.bind(this), | ||
| false); |
There was a problem hiding this comment.
According to https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters, false is the default here, so can't we just remove that?
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
hand-tool-class
branch
from
3170fa2
to
4cbb872
Compare
| function HandTool(options) { | ||
| this.container = options.container; | ||
| this.toggleHandTool = options.toggleHandTool; | ||
|
|
There was a problem hiding this comment.
Since we usually add all the various members, with suitable defaults, when initializing classes, you may want to add this.wasActive here as well. Please refer to the {enter,exit}PresentationMode code.
There was a problem hiding this comment.
Done in the new commit. It was also a bit weird that the value of the member variable was either true or null, so I changed that to true and false.
|
You need to update https://github.com/mozilla/pdf.js/blob/master/web/app.js#L2151 as well, otherwise pressing H throws an error. r=me, with the above comments and questions answered/fixed. Thank you for doing this! (Also, I'd really like to get rid of e.g. the |
timvandermeij
force-pushed
the
hand-tool-class
branch
from
4cbb872
to
8812ca5
Compare
|
The shortcut line has also been updated. |
timvandermeij
force-pushed
the
hand-tool-class
branch
from
8812ca5
to
b0aca31
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/a2ec67e382692be/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/a2ec67e382692be/output.txt Total script time: 0.98 mins Published |
|
Thank you for reviewing this, @Snuffleupagus! |
Furthermore we add more comments to the file.