-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[api-minor] Further modernize the ProgressBar
class (PR 14918 follow-up)
#15121
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
Conversation
…w-up) - Simplify how we look-up the DOM-element, which should also be a tiny bit more efficent. - Use private class-fields, rather than property-names prefixed with underscores. - Inline the `#updateBar` helper-method directly in the `percent`-setter, since having a separate method doesn't seem necessary in this case. - Set the `indeterminate`-class on the ProgressBar DOM-element, to simplify the code. Finally, also (slightly) re-factors the `PDFViewerApplication.progress`-method to make it a bit smaller.
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/22c9ce6375eccb5/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1568272d7014980/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/1568272d7014980/output.txt Total script time: 4.73 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/22c9ce6375eccb5/output.txt Total script time: 7.63 mins
|
@@ -718,7 +718,7 @@ const PDFViewerApplication = { | |||
}, | |||
|
|||
get loadingBar() { | |||
const bar = new ProgressBar("#loadingBar"); | |||
const bar = new ProgressBar("loadingBar"); | |||
return shadow(this, "loadingBar", bar); |
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 is unrelated to this patch, does it make sense to shadow the value ?
I suppose that once the document is loaded we don't need it anymore.
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 is unrelated to this patch, does it make sense to shadow the value ?
Doing that simplifies the code overall, and also allows us to initialize it lazily, since we're accessing it from multiple spots throughout the viewer code; see e.g.
Lines 1163 to 1164 in 13c01b6
if (percent > this.loadingBar.percent || isNaN(percent)) { this.loadingBar.percent = percent; Lines 1180 to 1183 in 13c01b6
this.loadingBar.show(); this.disableAutoFetchLoadingBarTimeout = setTimeout(() => { this.loadingBar.hide(); Line 1196 in 13c01b6
this.loadingBar.hide(); Line 1255 in 13c01b6
this.loadingBar.setWidth(this.appConfig.viewerContainer);
I suppose that once the document is loaded we don't need it anymore.
Correct; however if we'd remove it at the end of loading that'd (as far as I can tell) either mean having to initialize it eagerly or possibly require adding more code.
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.
LGTM. Thank you
Simplify how we look-up the DOM-element, which should also be a tiny bit more efficent.
Use private class-fields, rather than property-names prefixed with underscores.
Inline the
#updateBar
helper-method directly in thepercent
-setter, since having a separate method doesn't seem necessary in this case.Set the
indeterminate
-class on the ProgressBar DOM-element, to simplify the code.Finally, also (slightly) re-factors the
PDFViewerApplication.progress
-method to make it a bit smaller.