-
Notifications
You must be signed in to change notification settings - Fork 801
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
Conversation
This is an automated check which relies on E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16107 |
d8f276e
to
b45cda3
Compare
Thanks, Jan for your feedback.
Good point, you're right. 🤦
👍
yeah, in theory. But it's handled by the core component. Going to research a little bit more about this to improve the behavior.
👍
Me neither 🤷♂️ |
…tpack into update/cover-block-upgrade
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. |
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. |
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.
Quick scan through. Some minor points. Will do a test and discuss the options for the "Replace" issue you've reported.
c1f97e2
to
c272350
Compare
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.
This works well in my tests. 🚢
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.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:
/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.
Try to upload a weird file. You should see the weird message. It's handled by the server-side:
Try to upload an image. Everything should be fine.
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: