-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix: Preview well-formed file in offline (fixes #793) #799
Fix: Preview well-formed file in offline (fixes #793) #799
Conversation
Hi @stas-dolgachov, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks! |
CLA signed |
src/lib/Preview.js
Outdated
getTokens(this.file.id, this.previewOptions.token) | ||
.then(this.handleTokenResponse) | ||
.catch(this.handleFetchError); | ||
const couldPreviewOfflineWithoutToken = typeof fileIdOrFile === 'object' && this.options.skipServerUpdate; |
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.
Can you rename this to something starting with is? maybe isOfflinePreview
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.
Thank you, done
src/lib/Preview.js
Outdated
@@ -798,6 +803,9 @@ class Preview extends EventEmitter { | |||
|
|||
// Load from cache if the current file is valid, otherwise load file info from server | |||
if (checkFileValid(this.file)) { | |||
/* Save file in cache. This also adds the 'ORIGINAL' representation. |
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.
Change comment style to use //
instead of /*
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.
Thank you, done
src/lib/__tests__/Preview-test.js
Outdated
@@ -1002,6 +1002,15 @@ describe('lib/Preview', () => { | |||
expect(preview.retryTimeout).to.equal(undefined); | |||
}); | |||
|
|||
it('should load preview when a well-formed file object is passed and server update should be skipped', () => { | |||
const previewOptions = { skipServerUpdate: true }; | |||
preview.parseOptions(previewOptions); |
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.
Since we are unit testing load
, you shouldn't call parseOptions. Instead set the required fields on the preview object
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.
Thank you, done.
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.
Thanks so much for fixing this!
@stas-dolgachov: One thing I would note is that if your use case is to allow users to preview files offline, you will probably also need to set the preview option disableEventLog
to true. This will disable Preview event logging requests.
skipServerUpdate
only prevents the load from server and any server updates.
Hello guys,
This pull request fixes issue - #793
To remind you, it is required to preview well-formed files in offline mode without internet connection.
I am looking forward getting a feedback from you.
Thank you in advance