-
Notifications
You must be signed in to change notification settings - Fork 808
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
Media Library: Implement upload media from URL #41089
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
85afb83
to
24a4df4
Compare
3f8e1cb
to
ba93666
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.
Tested this both on Media -> Library and Site Editor and worked well 👍
...cts/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload-form/index.jsx
Outdated
Show resolved
Hide resolved
|
||
import './style.scss'; | ||
|
||
const WpcomMediaUrlUploadForm = ( { ajaxUrl, action, nonce, isSiteEditor } ) => { |
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.
It seems that
Screen.Capture.on.2025-01-22.at.17-12-33.mp4 |
...cts/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload-form/index.jsx
Outdated
Show resolved
Hide resolved
projects/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload.php
Outdated
Show resolved
Hide resolved
...cts/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload-form/index.jsx
Outdated
Show resolved
Hide resolved
It's all based on the design in https://github.com/Automattic/dotcom-forge/issues/10180#issuecomment-2572909885. @jameskoster, thoughts? |
Yep but the design doesn't have the storage usage there. Maybe we can remove it since there is another one below the page title 🤔 |
f6c24d0
to
d37ff21
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.
I could check the visual updates 👍
...cts/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload-form/index.jsx
Outdated
Show resolved
Hide resolved
But isn't this the whole reason of the PT? This is to (re-)allow users to upload images from URL in the Site Editor, because without the editor iframe, we lose the ability to do that using native Core's Media Library (see: Automattic/wp-calypso#98590) |
d37ff21
to
cc6bb9f
Compare
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
@jameskoster, thoughts? Should we allow the user to "close" the form and show the link back? (I'm not sure if it's worth it IMO) |
1fcbc2a
to
984679d
Compare
I'm not sure it's a good idea. The Calypso version of this page shows this storage info in all cases. If we remove it, I'm afraid users will lose this functionality.
Unfortunately, in the Site Editor (media blocks), it's the only place where we show the storage info, so we can't remove it.
This is done ✅
Unfortunately, the link color comes from Core (https://github.com/WordPress/wordpress-develop/blob/bcdca3f9925f1d3eca7b78d231837c0caf0c8c24/src/wp-includes/css/media-views.css#L39) with very high specificity of the selector. It's not easy to override it without JavaScript magic because there's no way to get the current link color of the current color scheme 🥲 To overcome the above issue(s), what if we make it as a button, and put it above the storage info? Like this: I pushed it to this PR. What do you think? |
How about using the |
What's the error message you see if you run out of space and try to upload media via blocks? |
@lsl Looking at the code I think it will just show this generic message like this if this is what you're asking. |
I'm now thinking that we should treat this discussion as separate ticket (like the upsell message simplification). It's not really related to the upload via URL feature 🥹 |
Make sense! I'll review this PR again, and let's iterate the UI in the follow-up PR 🙂 |
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.
Looks good to me. Left some minor feedback 🙂
...ts/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload-form/style.scss
Outdated
Show resolved
Hide resolved
projects/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload.php
Outdated
Show resolved
Hide resolved
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.
Tested and works as expected 👍
Fixes https://github.com/Automattic/dotcom-forge/issues/10180
Proposed changes:
This PR implements the feature where we can upload media files from URL in the Media Library and Site Editor.
The UX is implemented by following the default file upload behavior as similar as possible.
See the following videos:
Media Library
48MySt.mp4
Site Editor
lFwwGW.mp4
Implementation Details
This feature is implemented by showing a simple url input form after the
Plupload
uploader via thepost-plupload-upload-ui
hook. On submitting the form, it calls an AJAX handler, which then call Core'sdownload_url()
and thenmedia_handle_sideload()
. This logic is very similar to that of the original endpoint used by Calypso version of the Media Library.Limitations
Failure Cases
The upload form can handle failure cases, e.g. when the media has unsupported extension. It will show an error notice, similar to when we upload the file via the dropzone:
PwQbJv.mp4
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: