Convert the secondary toolbar to a class #7313
Conversation
timvandermeij
force-pushed
the
secondary-toolbar-class
branch
4 times, most recently
from
c71b6c1 to
41e7a0a
Compare
|
|
||
| // Bind the event listeners for click and hand tool actions. | ||
| this.bindClickListeners(); | ||
| this.bindHandToolListener(); |
There was a problem hiding this comment.
I'll try to do a proper review later on, but just a drive-by comment so far: Let's make these methods "private", i.e. this._bind... (with the associated JSDoc comments).
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
secondary-toolbar-class
branch
from
41e7a0a to
26dc176
Compare
| this.toolbar = options.toolbar; | ||
| this.buttonContainer = this.toolbar.firstElementChild; | ||
|
|
||
| // Define the toolbar buttons. | ||
| this.toggleButton = options.toggleButton; | ||
| this.presentationModeButton = options.presentationModeButton; |
There was a problem hiding this comment.
This is a minor (existing) inconsistency, where some of the options have ...Button on the end, and others don't. This is likely to be for "historical reasons", but since you're touching this code anyway, it'd be nice to make this more consistent :-)
There was a problem hiding this comment.
Done in the new commit.
There was a problem hiding this comment.
Thanks for doing this!
However, looking at the updated patch, the "Button" suffix everywhere makes the code look a bit busy. So, do we actually need it, or could we remove it for all SecondaryToolbar buttons?
I'm really sorry about not suggesting this previously, so if you do not want to remove the "...Button" part from all strings in this patch, I will not insist on this change!
There was a problem hiding this comment.
I think that the Button suffix looks better, especially when comparing cases such as this.print versus this.printButton and this.download versus this.downloadButton. For me, the Button suffix makes it more clear that it is actually a reference to a button object, instead of an action like printing or downloading. I do see that this is a personal preference though.
To resolve the concern somewhat, I managed to get rid of the separate member variables for the buttons altogether by moving the button array to the constructor. This makes it look cleaner because we do not have to mention all the buttons twice anymore. I only had to pass in the hand tool button to the _bindHandToolListener method as that was the only method that actually used one of the button member variables.
Fun fact: after this change we have exactly +178 −178 line changes for this patch :-)
timvandermeij
force-pushed
the
secondary-toolbar-class
branch
4 times, most recently
from
eb7f807 to
ad415c9
Compare
| * @property {HTMLButtonElement} toggleHandToolButton - Button to toggle the | ||
| * hand tool. | ||
| * @property {HTMLButtonElement} documentPropertiesButton - Button to toggle the | ||
| * visibility of the document properties overlay. |
There was a problem hiding this comment.
This isn't really a toggle, so e.g. "Button for opening the document properties dialog" might be better here.
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
secondary-toolbar-class
branch
from
ad415c9 to
f4ae277
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/caa752fa9224791/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/caa752fa9224791/output.txt Total script time: 1.05 mins Published |
That's really nice! This is looking much better now, thank you for the patch (and patience)! |
|
No worries, I'm really happy with how this turned out after your comments! |
Furthermore we:
This patch is easier to review with the
?w=1flag.