-
Notifications
You must be signed in to change notification settings - Fork 58
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
Initial support for VideoPress v5 #6634
Merged
Merged
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
1d42425
Update Jetpack reference
0a56e6a
Update with bundle changes
b0278b6
Update Jetpack reference
37609f0
Update Jetpack reference
139d90e
Update Jetpack reference
9485b3b
Update Jetpack reference
a837182
Update Jetpack reference
ce21e69
Update Jetpack reference
4369bc9
fix: Limit VideoPress v5 to supported sites
c5e59b2
Update Jetpack reference
3479eea
refactor: Be more specific with capability
897f363
Update Gutenberg reference
d521cb2
Update Jetpack reference
8a55c0a
Update RELEASE-NOTES
e4abec2
Merge branch 'trunk' into jetpack/support-for-videopress-v5
049f383
Update Jetpack reference
fluiddot bc69c52
Update localization strings files
fluiddot 79d1f01
Merge branch 'trunk' into jetpack/support-for-videopress-v5
393ad23
Fix duplicated release note
07f5e24
Update Gutenberg reference
9f223de
Update Jetpack reference
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Submodule gutenberg
updated
4 files
Submodule jetpack
updated
1461 files
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,6 @@ import { | |
registerLoomVariation, | ||
registerSmartframeVariation, | ||
} from '../jetpack/projects/plugins/jetpack/extensions/extended-blocks/core-embed'; | ||
import '../jetpack/projects/plugins/jetpack/extensions/blocks/videopress/editor'; | ||
|
||
// When adding new blocks to this list please also consider updating `./block-support/supported-blocks.json` | ||
const supportedJetpackBlocks = { | ||
|
@@ -129,6 +128,14 @@ export function registerJetpackEmbedVariations( { capabilities } ) { | |
} ); | ||
} | ||
|
||
export function enableVideoPressV5Support( { capabilities } ) { | ||
if ( ! isActive() || ! capabilities.videoPressV5Support ) { | ||
return; | ||
} | ||
|
||
require( '../jetpack/projects/plugins/jetpack/extensions/blocks/videopress/editor' ); | ||
} | ||
Comment on lines
+131
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a side note, it would be great if we could cover this logic in a test case here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, I'll add this to my list to follow up on next week. |
||
|
||
const setupHooks = () => { | ||
// Hook triggered before the editor is rendered | ||
addAction( 'native.pre-render', 'gutenberg-mobile-jetpack', ( props ) => { | ||
|
@@ -142,6 +149,11 @@ const setupHooks = () => { | |
// block type registration, so it’s required to add them before | ||
// the core blocks are registered. | ||
registerJetpackEmbedVariations( props ); | ||
|
||
// VideoPress v5 conversion also uses WP hooks that are attached to | ||
// block type registration, so it’s required to add them before | ||
// the core blocks are registered. | ||
enableVideoPressV5Support( props ); | ||
} ); | ||
|
||
// Hook triggered after the editor is rendered | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The following strings are duplicated on Android, however, they are not on iOS. Seems that form some reason, the i18n update scripts are identifying these strings as new. I'd like to explore this further but in the meantime, I wouldn't block the PR due to this.
Edit video
Video caption. Empty
Video caption. %s
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.
One option I'm considering is that since they are associated with the Jetpack plugin, they should be considered different from the ones provided in Gutenberg. However, I noticed that something similar is happening on the Jetpack VideoPress plugin (example) and they weren't marked as new 🤔.
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.
Thank you for flagging this, Carlos. I'm going to go ahead to begin the merge domino for this PR, and it's associated changes as you mentioned this is non-blocking. But, will be happy to help look further into this specific issue with you next week.