Update: Continuous progress bar through retries #883
Conversation
ConradJChan
commented
Dec 27, 2018
ConradJChan
commented
|
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
src/lib/Preview.js
Outdated
| @@ -814,6 +814,11 @@ class Preview extends EventEmitter { | |||
| * @return {void} | |||
| */ | |||
| setupUI() { | |||
| // Do nothing if container is already setup and in the middle of retrying | |||
| if (this.container && this.retryCount > 0) { | |||
There was a problem hiding this comment.
would this.open be helpful here?
There was a problem hiding this comment.
I'm not sure, it seems like this.open is set when load is first called and is no guarantee of the preview UI having been setup already
src/lib/Preview.js
Outdated
| @@ -814,6 +814,11 @@ class Preview extends EventEmitter { | |||
| * @return {void} | |||
| */ | |||
| setupUI() { | |||
| // Do nothing if container is already setup and in the middle of retrying | |||
| if (this.container && this.retryCount > 0) { | |||
There was a problem hiding this comment.
This seems like a somewhat risky place for this change, since the this.ui.setup method is intended to destroy/replace the existing UI. Does this cover edge cases where a particular file is being retried and the user navigates to another file?
There was a problem hiding this comment.
Looking at the code for navigating prev/next files, maybe it would be better to use retryTimeout instead of relying on retryCount since I don't see that getting reset in the navigation flow.
The other approach I was thinking was to pass in a retry boolean to be more explicit when calling load via handleFetchError but I would need to propagate that value through a few functions and result in a bigger change, let me take another look
src/lib/Preview.js
Outdated
| @@ -814,6 +814,11 @@ class Preview extends EventEmitter { | |||
| * @return {void} | |||
| */ | |||
| setupUI() { | |||
| // Do nothing if container is already setup and in the middle of retrying | |||
| if (this.container && this.retryCount > 0) { | |||
There was a problem hiding this comment.
It doesn't look like this.container gets cleared out in the destroy or navigate, so this may cause issues
There was a problem hiding this comment.
created the method PreviewUI.isSetup() instead
| this.setupUI(); | ||
| // Do nothing if container is already setup and in the middle of retrying | ||
| if (!(this.ui.isSetup() && this.retryCount > 0)) { | ||
| this.setupUI(); |
There was a problem hiding this comment.
Do we want to add this to the reload case? We call it a few times throughout preview.js
There was a problem hiding this comment.
I'm not sure, IMO the reload intention is to wipe everything clean and start over from scratch in which case, it seems to make sense to always re setup the UI
