Skip to content

feat: add freemium upsell experiment changes - #2739

Merged
jackiejou merged 12 commits into
box:masterfrom
jackiejou:add-exp-freemium-upsell-components
Nov 5, 2021
Merged

feat: add freemium upsell experiment changes#2739
jackiejou merged 12 commits into
box:masterfrom
jackiejou:add-exp-freemium-upsell-components

Conversation

@jackiejou

Copy link
Copy Markdown
Contributor

With Experiment Enabled. Upgrade Button
image

Without Experiment Enabled.
image

@jackiejou
jackiejou requested review from a team as code owners November 3, 2021 18:39
Comment thread src/elements/content-uploader/ItemList.js
};

const UploadsManager = ({
freemiumUpsellCTACb,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

love this re-alphabetization

Comment thread src/elements/content-uploader/ItemList.js Outdated
case ERROR_CODE_UPLOAD_CHILD_FOLDER_FAILED:
return <FormattedMessage {...messages.uploadsOneOrMoreChildFoldersFailedToUploadMessage} />;
case ERROR_CODE_UPLOAD_FILE_SIZE_LIMIT_EXCEEDED:
if (maxFileSize) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to have two sources of truth (client, server) for the maximum file size. Can we add a new error code on the backend that we can key off of to ask the user to upgrade, instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The plan is to get the maxFileSize from the server in the EUA (exist as CurrentUserRecord) rather than passing in a hardcoded data in the FE repo. This avoids adding new endpoint/changing current APIs for this experimental change. (reference maxFileSizeAsString in EUA)

measureRef,
isTouch,
fileLimit,
onFreemiumUpsellCTAClick,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ContentUploader can used by platform developers in their own applications, outside of Box's webapp. We should try to keep the component API simple and general, when possible. Do we plan to add more experiences like this one? If so, should we wrap them in an object to avoid further prop bloat?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are the final current planned ContentUploader changes for this project. No other prop needed. I suspect that once the experiment concludes, I will make a PR to clean up these changes (2 props)

}

if (STATUS_ERROR && code === ERROR_CODE_UPLOAD_FILE_SIZE_LIMIT_EXCEEDED && !!onFreemiumUpsellCTAClick) {
return (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We generally try to avoid multiple returns, as it can lead to confusing behavior. Can we tie into the status switch above and the existing onClick handler, instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The PrimaryButton design (a new resin tag, no tooltip) is quite different from the current Plain Button icon with tooltip. Classname bcu-item-action also limits the width and height of this component which would cause the PrimaryButton to be cut off or misaligned. In addition, code clean up may be easier in the future.

Comment thread src/elements/content-uploader/ItemList.js
Comment thread src/elements/content-uploader/progressCellRenderer.js Outdated
@jackiejou
jackiejou merged commit b412e29 into box:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants