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.

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

Cover: Handling site plan and its errors #16107

Merged
merged 33 commits into from
Jun 15, 2020
Merged

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Jun 9, 2020

This PR extends the MediaPlaceholder component for the core/cover block, to improve the messages to the users when try to upload different files, depending on the current site plan.

  • not simple site: no changes
  • paid plan: no changes
  • simple site: free plan
    • The user tries to upload an image: no changes
    • The user tries to upload a no video file: no changes. It shows the regular error message
    • The user tries to upload a video: changes!!! It shows the upgrade nudge.

For the last condition, it worths mentioning it works either dropping the file as well as by clicking on the upload button.

Changes proposed in this Pull Request:

  • It adds a new (controversial) /blocks folder in /share. After testing I decided to put it there because it isn't a Jetpack block but is a core one. And also, it modifies the functionality of some pieces (and I think we are going to keep doing this soon). In this case, the MediaPlaceholder component.

Testing instructions:

Jetpack site
Nothing should change. Just create a cover block and update a video. It should work as usual.

Simple site

  • Get Sandboxed

  • Apply D44715-code

  • Try to upload a video for a free site using the media placeholder. Confirm that it shows the upgrade nudge.

image

  • Try to upload an unsupported file, for instance, a pdf. You should see the proper error message:

image

  • Try to upload a weird file. You should see the weird message. It's handled by the server-side:
    image

  • Try to upload an image. Everything should be fine.
    image

  • Test using either the Upload button as well as dropping files in the placeholder area.

  • Confirm the video block looks as exected. This PR touches shared code used by this component.

Proposed changelog entry for your changes:

  • Handling improvement for video files in the core/cover block.

@jetpackbot
Copy link

jetpackbot commented Jun 9, 2020

Warnings
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16107

Generated by 🚫 dangerJS against c272350

@retrofox retrofox force-pushed the update/cover-block-upgrade branch from d8f276e to b45cda3 Compare June 9, 2020 21:53
@retrofox retrofox changed the title [wip] Update/cover block upgrade Cover: Handling site plan and its errors Jun 10, 2020
@retrofox retrofox marked this pull request as ready for review June 10, 2020 11:29
@retrofox retrofox requested a review from a team June 10, 2020 11:30
@retrofox retrofox added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it Plans [Feature] VideoPress A feature to help you upload and insert videos on your site. [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 10, 2020
@retrofox
Copy link
Contributor Author

Thanks, Jan for your feedback.

  • For the Cover Block, I don't think we should add the subheading/description "... unbranded, customizable player" since the Cover Block doesn't actually display video player controls, does it?

Good point, you're right. 🤦

We can keep the messaging simple and drop the description similar to the one mentioned here: To use a video in this block, upgrade to WordPress.com Premium.

👍

Should this not have the "file type not supported error" still? Since they're still attempting to upload a certain type of file.

yeah, in theory. But it's handled by the core component. Going to research a little bit more about this to improve the behavior.

Related feedback:

  • I would make the upsell heading the same for both Video and Cover Blocks.

👍

  • I didn't realize a user can actually customize the video player?

Me neither 🤷‍♂️

@retrofox retrofox added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 10, 2020
@retrofox retrofox added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 10, 2020
@github-actions
Copy link
Contributor

Howdy! The Jetpack team has disappeared for a few days to recharge our batteries. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back on Monday, June 15 to look at your work! Thank you for your understanding.

@retrofox
Copy link
Contributor Author

Should this not have the "file type not supported error" still? Since they're still attempting to upload a certain type of file.

For some reason, the core component doesn't detect it on the client-side. It performs a request to the server-side to finally return an error. But, we'll be able to deal with this soon since we've merged a patch in the core which will allow us to know the files before to start the uploading process.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Quick scan through. Some minor points. Will do a test and discuss the options for the "Replace" issue you've reported.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well in my tests. 🚢

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 15, 2020
@kraftbj kraftbj dismissed getdave’s stale review June 15, 2020 19:15

ready to merge

@kraftbj kraftbj merged commit d0d2810 into master Jun 15, 2020
@kraftbj kraftbj deleted the update/cover-block-upgrade branch June 15, 2020 19:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. Plans Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants