Skip to content
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.

Sign up for GitHub

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

Update: Continuous progress bar through retries #883

Merged
merged 2 commits into from
Jan 9, 2019
Merged

Update: Continuous progress bar through retries #883

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b545f9a
Update: Continuous progress bar through retries
Dec 27, 2018
8e6df2e
Chore: addressing PR comments
Dec 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/lib/Preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this.open be helpful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this.container gets cleared out in the destroy or navigate, so this may cause issues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created the method PreviewUI.isSetup() instead

return;
}

// Setup the shell
this.container = this.ui.setup(
this.options,
Expand Down
41 changes: 41 additions & 0 deletions src/lib/__tests__/Preview-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,47 @@ describe('lib/Preview', () => {

preview.setupUI();
});

it('should setup everything even though container is not falsy', () => {
const previewUIMock = sandbox.mock(preview.ui);
previewUIMock.expects('setup');
previewUIMock.expects('showLoadingIndicator');
previewUIMock.expects('startProgressBar');
previewUIMock.expects('showNavigation');
previewUIMock.expects('setupNotification');

preview.container = {};

preview.setupUI();
});

it('should setup everything even though retryCount is > 0', () => {
const previewUIMock = sandbox.mock(preview.ui);
previewUIMock.expects('setup');
previewUIMock.expects('showLoadingIndicator');
previewUIMock.expects('startProgressBar');
previewUIMock.expects('showNavigation');
previewUIMock.expects('setupNotification');

preview.container = undefined;
preview.retryCount = 1;

preview.setupUI();
});

it('should not setup anything if container is truthy and retryCount > 0', () => {
const previewUIMock = sandbox.mock(preview.ui);
previewUIMock.expects('setup').never();
previewUIMock.expects('showLoadingIndicator').never();
previewUIMock.expects('startProgressBar').never();
previewUIMock.expects('showNavigation').never();
previewUIMock.expects('setupNotification').never();

preview.container = {};
preview.retryCount = 1;

preview.setupUI();
});
});

describe('parseOptions()', () => {
Expand Down