-
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
[RNMobile] Prevent validation error when viewing VideoPress markup within app #24548
Conversation
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. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Caution: This PR has changes that must be merged to WordPress.com |
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, nice work!
Thank you, Jason! Marking this as ready for review by the Jetpack crew now. :) Note for the crew, I don't have commit access for WP.com, so would be grateful if you could handle committing to WP.com for me too. 🙇♀️ |
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'll trust the team review on this one and rubber-stamp it, since all this does is add a native file that we don't use ourselves.
Great news! One last step: head over to your WordPress.com diff, D81711-code, and deploy it. Thank you! |
r247543-wpcom |
Hi @SiobhyB 👋 😄 . Thank you for your work on this issue!
Because of the way that the |
@mkevins, thank you for checking in and highlighting that issue! I don't believe the changes from this specific PR have been released into the app yet. They're set to be including in the upcoming The changes from this PR should only override the For the user report you linked to: I'll spend a bit of time exploring that either today or tomorrow and reply with thoughts on that GitHub issue. Thank you so much again for highlighting! And please do let me know if I'm missing any specific issues with the code from this PR. |
@SiobhyB 👋 😃
I didn't mean to suggest that this PR impacted the uploads, so my apologies for not making this more clear in my comment 😅 . Rather, I noticed that you mentioned that this PR doesn't address the upload scenario, so I was hoping you might have some more context about that. The issue I linked to (a different one than this PR partially solves) seems to be reproducible with some site types (non-atomic business, and possibly also premium sites, for example). tl;dr of that issue is that when the user uploads from the mobile editor, the upload actually completes successfully (at least, the video makes it into the WordPress Media Library), but the remote When I found this PR, I saw that it mostly addresses a separate (related) issue: that the content generated on web is parsed as invalid (and possibly modified?) on mobile - seemingly an issue that has been around for a long time 😅 . My hope was that perhaps you might know more about the different way that URLs are handled in the VideoPress block vs. Video block - but no worries if not. Hopefully that clarifies things a bit? |
@mkevins, aha! My apologies for jumping to conclusions and misunderstanding your question. 🤦♀️ Thank you for clarifying. I've now read through both the user's ticket and the Slack thread where you discussed with HEs for context. I've attempted to outline the context for this PR and my thoughts on the user's issues further below, let me know if anything is unclear! Context for this PRThe app doesn't support VideoPress at the moment. So, any video uploaded via the app should have a plain The web is different. Videos uploaded to WordPress.com on the web will have a VideoPress URL associated with them. The code in this file make some changes to the core video block as part of the upload/VideoPress support. If a post containing a core video block is saved on the web and the site has VideoPress enabled (e.g. WordPress.com sites), the code in this file will automatically convert the code to use VideoPress, just as it would if you'd uploaded directly to the web. This is what was causing a validation error within the app, due to the lack of VideoPress support. This PR prevents the in-app validation error but video uploads via the app still aren't in alignment with the web in terms of VideoPress support. Video uploads in the app will still be uploaded to the unmodified core video block using the plain More details around this PR: p9ugOq-2W8-p2 The user's issueI attempting to reproduce using a simple site with a Business plan, but the video thumbnail appeared as expected and I was able to play it directly from the editor. 🤔 Looking over the thread, I noticed that some of the videos appeared to be VideoPress videos. Do you know if those were uploaded via the app or the web? |
Hi @SiobhyB 👋 😃 , thank you for your detailed reply!
Yes, this does make sense. Thanks for elaborating - this confirms my understanding of the PR changes. Also, thank you for that link!
I'm pretty sure that these were uploaded via the mobile editor - but I have not been able to reproduce this myself, since my Business test site is an atomic test site. Afaik, this has been reproducible on a Business site without any plugins. Do you know whether the site you tried to reproduce this has any plugins (i.e. atomic?). Another thing that was interesting in the thread where this was reproduced (p1657862661885529-slack-C03URUK5C), is that the URLs were different when inserted via the editor, depending on whether they were updated as part of upload completion, or if they were already uploaded and part of the Media Library. When you mention I wonder, have you encountered URLs like this while exploring / testing this? I'm curious, because, if so, and you have not encountered the 403 error, perhaps there is some other issue at the root of that. If not, I think it's worth further investigating why the URLs can sometimes be different. |
Partially fixes WordPress/gutenberg#30987
At the moment, blocks with VideoPress markup create a validation error within the app. This creates a problem as posts that contain videos (for sites with VideoPress enabled) are automatically converted to use VideoPress markup when saved on the web.
Related PRs
gutenberg-mobile
: Prevent validation error when viewing VideoPress markup within app wordpress-mobile/gutenberg-mobile#4899Installable builds
android
: [TESTING ONLY] Prevent validation error when viewing VideoPress markup within app wordpress-mobile/WordPress-Android#16664iOS
: [TESTING ONLY] Prevent validation error when viewing VideoPress markup within app wordpress-mobile/WordPress-iOS#18775Changes proposed in this Pull Request:
editor.js
file "intercepts" the video block to make some modifications when saved. With this PR, a native version of theeditor.js
file has been created to enable us to make our own native-specific modifications.save.js
file to prevent a validation error when viewing a block with VideoPress markup within the app.Screenshots
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
N/A
Testing instructions:
WordPress.com site
Self-hosted site without Jetpack
In addition to confirming that the original bug is address, we should also confirm that there are no regressions to the way the video block appears for sites hosted outside of WordPress.com and without Jetpack.