New: Watermarking preferences #721
Conversation
tonyjin
requested review from
DanDeMicco,
JustinHoldstock,
jeremypress and
pramodsum
|
Verified that @tonyjin has signed the CLA. Thanks for the pull request! |
tonyjin
force-pushed
the
watermarking-pref
branch
from
9d36ad9 to
ae4c7c9
Compare
| if (!canDownload(this.file, this.options)) { | ||
| this.ui.showNotification(downloadErrorMsg); |
There was a problem hiding this comment.
Why would we show a button if they can't download? If we don't know till reps come back this will get interesting...
There was a problem hiding this comment.
This is a public method so one could call preview.download() directly
src/lib/Preview.js
Outdated
| } | ||
| }); | ||
|
|
||
| queryParams.response_content_disposition_type = 'attachment'; |
There was a problem hiding this comment.
What does setting this to attachment accomplish?
There was a problem hiding this comment.
You had this in your original PR to force Content Disposition to attachment instead of the default (inline?). I assume this came out of a conversation between you and Albert.
There was a problem hiding this comment.
If you decide to keep this line, I think you should do an object spread / Object.assign for this. If you add this here, you will be modifying the options the user passes in, and all further uses of this.options.queryParams will have response_content_disposition_type which may not be intended
There was a problem hiding this comment.
@tonyjin yeah I think that came from him as a way to force the browser to treat it like a DL. We should experiment if we can replace opening in the iframe with that?
There was a problem hiding this comment.
I'll be removing response_content_disposition_type since we don't need it, but I'll definitely be using Object.assign other places instead of directly modifying queryParams. Thanks for the reminder @DanDeMicco.
| // are at least a `Viewer-Uploader` and no preview otherwise. | ||
| // 'none' - Forces non-watermarked previews. If the file type cannot be watermarked or the user is not at least | ||
| // a `Viewer-Uploader`, no preview is shown. | ||
| this.options.previewWMPref = options.previewWMPref || 'any'; |
There was a problem hiding this comment.
I don't think the "Pref" at the end adds much
There was a problem hiding this comment.
Wanted to distinguish some way between previewWM and downloadWM since one takes a string and one takes a boolean, but I'm open to dropping Pref
There was a problem hiding this comment.
ah makes sense, make the download one a boolean like verb by adding should or force to the front?
There was a problem hiding this comment.
+1 to prefixing the property name to help us distinguish boolean type
There was a problem hiding this comment.
I considered shouldDownloadWM and forceDownloadWM but I'm going to keep as downloadWM since download is already a verb and I interpret the option as downloadWatermarked, which implies true/false to me, whereas I interpret previewWMPref as previewWatermark(ed/ing)Preference, which implies options other than just true/false.
| * | ||
| * @return {Object} Box representation used/to be used by Preview | ||
| */ | ||
| getRepresentation() { |
tonyjin
force-pushed
the
watermarking-pref
branch
from
ae4c7c9 to
a0970e0
Compare
Add support for two new watermarking preferences: `previewWMPref` and `downloadWM`. `previewWMPref` allows configuration of the preview experience for watermarked files (do you see watermarked versions of the file or not) and `downloadWM` controls whether watermarked files or the original is downloaded via the download button and download() API. Also fixed some flaky tests.
tonyjin
force-pushed
the
watermarking-pref
branch
from
a0970e0 to
bc09eaa
Compare
Add support for two new watermarking preferences:
previewWMPrefanddownloadWM.previewWMPrefallows configuration of the preview experience for watermarked files (do you see watermarked versions of the file or not) anddownloadWMcontrols whether watermarked files or the original is downloaded via the download button and download() API.Also fixed some flaky tests.