-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Set a display value for the secondary toolbar buttons #15413
Conversation
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/4cb4e19403464d2/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/4cb4e19403464d2/output.txt Total script time: 2.08 mins Published |
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.
r=me, with comments addressed, since the actual code changes seem reasonable.
Personally I don't really understand why we need to move these buttons to the secondaryToolbar, given that there isn't exactly a lack of horizontal space on most modern screens, since I fear that in particular PresentationMode is something that users will file bugs/issues for asking where it went.
Also, as suggested in #15391 (comment), do we perhaps want to add telemetry (in a follow-up) to see how often PresentationMode is being used?
web/app.js
Outdated
@@ -811,8 +811,7 @@ const PDFViewerApplication = { | |||
*/ | |||
_hideViewBookmark() { | |||
// URL does not reflect proper document location - hiding some buttons. | |||
const { toolbar, secondaryToolbar } = this.appConfig; | |||
toolbar.viewBookmark.hidden = true; | |||
const { secondaryToolbar } = this.appConfig; | |||
secondaryToolbar.viewBookmarkButton.hidden = true; |
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.
With the changes here, we could just directly do the following instead:
secondaryToolbar.viewBookmarkButton.hidden = true; | |
this.appConfig.secondaryToolbar.viewBookmarkButton.hidden = true; |
web/viewer.html
Outdated
<button id="presentationMode" class="toolbarButton hiddenLargeView" title="Switch to Presentation Mode" tabindex="31" data-l10n-id="presentation_mode"> | ||
<span data-l10n-id="presentation_mode_label">Presentation Mode</span> | ||
</button> | ||
|
||
<!--#if GENERIC--> | ||
<button id="openFile" class="toolbarButton hiddenLargeView" title="Open File" tabindex="32" data-l10n-id="open_file"> |
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.
Shouldn't these tabindex
s, for the buttons in the toolbarViewerRight
-div, be renumbered starting from 31
instead now?
(Since there's also a "hole" in the numbering between the "download" and the "editorFreeText" buttons now.)
d72f664
to
10c89c2
Compare
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/76e95f631634520/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1e49894cb2b2a60/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/1e49894cb2b2a60/output.txt Total script time: 4.92 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/76e95f631634520/output.txt Total script time: 9.73 mins
|
web/app.js
Outdated
const { toolbar, secondaryToolbar } = this.appConfig; | ||
toolbar.viewBookmark.hidden = true; | ||
secondaryToolbar.viewBookmarkButton.hidden = true; | ||
this.appConfig.viewBookmarkButton.hidden = true; |
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.
I believe that this will throw, since it's not exactly what I suggested in #15413 (comment)
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.
pfff... my bad.
…lbar (bug 1789082)
and remove a useless property: visibleSmallView.
10c89c2
to
f8ae49d
Compare
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/692a79121c01bcd/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/57c6453c2d7a2e9/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/692a79121c01bcd/output.txt Total script time: 5.02 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/57c6453c2d7a2e9/output.txt Total script time: 9.65 mins
|
and remove a useless property: visibleSmallView.