-
Notifications
You must be signed in to change notification settings - Fork 809
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: Support Upload from URL on media-new page #41627
Media Library: Support Upload from URL on media-new page #41627
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! Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
document.addEventListener( 'DOMContentLoaded', function () { | ||
const pluploadUploadUI = document.getElementById( selectors.PLUPLOAD_UPLOAD_UI ); | ||
const selectFilesButton = document.getElementById( selectors.PLUPLOAD_BROWSE_BUTTON ); | ||
if ( pluploadUploadUI && selectFilesButton ) { |
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 think this is all too complicated. Can we try like this:
- We don't need to split
wpcom_media_url_upload()
intoappend_wpcom_media_url_upload()
andenqueue_wpcom_media_url_upload_form()
. - In media-new.php, we simply do
add_action( 'post-plupload-upload-ui', 'wpcom_media_url_upload' );
- In here, we just check
document.getElementById('wpcom-media-url-upload')
and then render the React component there.
I did the above before, but scratched it and decided not to implement it in media-new.php 😄
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 tried but the action is outside the of plupload-upload-ui
element 🙈
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.
Sorry I don't get it, could you explain more?
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.
Oh.... I see now. Thinking...
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.
OK, so I think we need to consider something like add_action( 'post-plupload-upload-ui', 'append_wpcom_media_url_upload', 30 );
.
This is because the text "You are using the multi-file uploader..." belongs to the uploader, so we should put other feature outside of it:
![image](https://private-user-images.githubusercontent.com/1525580/410885123-53430976-407e-416b-a3eb-bd9e4a53d6ae.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzODEwMTMsIm5iZiI6MTczOTM4MDcxMywicGF0aCI6Ii8xNTI1NTgwLzQxMDg4NTEyMy01MzQzMDk3Ni00MDdlLTQxNmItYTNlYi1iZDllNGE1M2Q2YWUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxMiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTJUMTcxODMzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzUyYmQxMTM0YjQ4NGRhMDcyYmQ2OWEwMDIwMDkwNWMxMGYwNGRlY2FhZjgzZDhiYjc4ZDhjN2IwZTI0MDM4OCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.JnTreHUsgasZDHHgbuvXnYnMaYeNWu_GmD1T2d70h8A)
Maybe we can change the text to "Or upload from URL" or change to a link as per original design.
What do you think? It's weird that the uploader behavior is different in this page 🤦
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.
Yep the page is totally different 🫠
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.
Maybe we can change the text to "Or upload from URL" or change to a link as per original design.
Sounds good to change to a link as per original design since we have already simplified the messages inside the form 👍
@@ -1,15 +1,32 @@ | |||
.wpcom-media-url-upload-form { | |||
margin-top: -15px; | |||
margin-bottom: 15px; | |||
height: 30px; |
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 needs to be here, otherwise when you click on the Upload button, the input field shows and the "You are currently using ..." text will shift to bottom.
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.
but it makes the UI weird on mobile 😓
@jameskoster We're adding the |
projects/packages/jetpack-mu-wpcom/src/features/wpcom-media/wpcom-media-url-upload.php
Show resolved
Hide resolved
} | ||
} | ||
|
||
#plupload-upload-ui.has-wpcom-media-url-upload { |
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 wonder why we need .has-wpcom-media-url-upload
? As far as I can tell, the #plupload-upload-ui
is only present in media-new.php
🤔
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 just want to keep it safe to apply styles to #plupload-upload-ui only when it has .has-wpcom-media-url-upload
. Maybe it's overthinking 🤔
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's perfect 😂
Related to p1738900618822769/1738898875.646109-slack-CRWCHQGUB
Proposed changes:
Upload from URL
button on the media-new page. It's a little hacky to display the upload status 🙈Upload from URL
link and allow users to toggle the form to align with the designScreenshots
Demo
media-new-upload-url.mov
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Image Block > Media Library > Upload from UR