VideoPress: add RN mobile video player block #29165
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:
Backup plugin:
Boost plugin:
Social plugin:
Starter Plugin plugin:
Protect plugin:
Videopress plugin:
Super Cache plugin:
Migration plugin:
Crm plugin:
Mu Wpcom plugin plugin:
|
|
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run to get started. More details: p9dueE-5Nn-p2 |
* [not verified] first approach of the poster panel * [not verified] export poster media types constant * [not verified] tweak poster image help section * [not verified] set background image to panel button * [not verified] add poster panel to the default settings tab * [not verified] changelog * Update projects/packages/videopress/src/client/block-editor/blocks/video/components/poster-panel/style.scss Co-authored-by: Douglas Henri <dhasilva@users.noreply.github.com> * [not verified] move poster action label to a const * two const > one const :smart: --------- Co-authored-by: Douglas Henri <dhasilva@users.noreply.github.com> Co-authored-by: Douglas <douglas.henri@automattic.com>
* [not verified] use tertiary and destructive style for remove btn * [not verified] do not use CSS module * [not verified] tweak delete button layout * changelog
…29173) * [not verified] use a const to define image styles dynamically * [not verified] adjust poster image height based on video ratio * changelog
* Exclude deferred JS in customizer preview * changelog --------- Co-authored-by: Mark George <thingalon@gmail.com>
…ement (#28403) Co-authored-by: Matt Gawarecki <matt.gawarecki@automattic.com>
| // Pick video properties from preview. | ||
| const { html: previewHtml, scripts } = preview; | ||
|
|
||
| const html = previewHtml || cacheHtml; |
There was a problem hiding this comment.
Regarding cacheHtml attribute, we'd need to replicate the same logic we have in the web version (reference).
There was a problem hiding this comment.
I'm not sure this is entirely true.
I did see the effect that sets the cacheHtml attribute in the web version, which you referenced, but I don't see where the attribute is actually being used in the video player component. For now I would suggest we keep the current logic scoped to what is needed to render the video player.
That said I find the comments above the effect hook puzzling and makes wonder if the cacheHtml attribute is actually needed. The comment mentions setting html and thumbnail attributes, neither of which are actually set in the effect hook.
I'll do some commit spelunking to find the reasoning for the setting the cacheHtml attribute, perhaps it is needed for the some of the other components like <VideoPressUploader> or <PlaceholderWrapper>
There was a problem hiding this comment.
I did see the effect that sets the cacheHtml attribute in the web version, which you referenced, but I don't see where the attribute is actually being used in the video player component. For now I would suggest we keep the current logic scoped to what is needed to render the video player.
I think the purpose of the cacheHtml attribute is to be used upon the component being mounted to display the video player while the preview is being fetched, as it's mentioned in this comment:
I understand that this is an enhancement to avoid displaying a loading indicator, which we might consider implementing in the native version. In any case, I tested this functionality in the web version and it's true that is hard to tell the difference with and without having the cache, especially as the HTML content is an iframe that still needs to load the video player content 🤔.
There was a problem hiding this comment.
I think the purpose of the cacheHtml attribute is to be used upon the component being mounted to display the video player while the preview is being fetched
Seems like the const html = previewHtml || cacheHtml assignment covers the mounting period in the block lifecycle.
This enhancement might be for when the preview is fetched after the component is mounted. The hook logic suggests that the previewHtml would have to exist then not exist before the setAttribute call would have any relevance.
I'm guessing that happens when the video source is updated. If so, we should probably handle that when we get to settings or uploading updates.
| return ( | ||
| <View | ||
| style={ { height: videoPlayerHeight } } | ||
| onLayout={ ( { nativeEvent } ) => scaleVideoPlayer( nativeEvent ) } |
There was a problem hiding this comment.
I recall that the height was automatically set in the Sandbox component (reference) communicating with the embed content, not sure if we could apply a similar approach for the VideoPress player 🤔 .
There was a problem hiding this comment.
I believe I was running into issues with the aspect ratio of the videos, not just the height. I'll play around with this to see if I can get the Sandbox to handle it correctly
There was a problem hiding this comment.
Ok I figured out what's going on here. I was following the web version a bit too closely I think. There is a "loading" view that needs some height. I think the web version also needs to handle responsive UI changes so I'm assuming the height needs to be handled explicitly.
I can handle the "is loading" state better which simplifies a lot of things.
Thanks for pointing that out!
| preview, | ||
| isRequestingEmbedPreview, | ||
| } ) { | ||
| const [ videoPlayerHeight, setVideoPlayerHeight ] = useState( 250 ); |
There was a problem hiding this comment.
In the web version, a temporary height is set, maybe we could replicate a similar logic instead of having a constant, WDYT?
There was a problem hiding this comment.
Are you referencing the magic number 250 ? That's just a replacement for the temporary styling height that was added here. I would expect that to be modified later once we get into refining the component. That said the web version also uses a constant, just not in the useState call. ( Just noticed while writing this that the web version now uses a constant as well )
Or are you talking the web version use of state.videoPlayerTemporaryHeight . It's really just naming. I've simplified the logic involved in calling setVideoPlayerHeight vs the web version setVideoPlayerTemporaryHeight but the intent is the same. I expect the calling logic to become more complex in future changes so I opted to keep it simple for now.
I found the use of temporary misleading in the web version. I couldn't find a condition where the "temporary" height is not used, it is ignored once the embed is rendered but it's not replaced or removed. Perhaps a better name, in both cases, is playerWrapperHeight since that is where how the state property is used.
There was a problem hiding this comment.
Are you referencing the magic number
250? That's just a replacement for the temporary styling height that was added here. I would expect that to be modified later once we get into refining the component. That said the web version also uses a constant, just not in theuseStatecall. ( Just noticed while writing this that the web version now uses a constant as well )
Yeah, I was referencing the magic number. Oh, true, in the web version we also have a constant. In that case, omit my comment then, thanks for elaborating!
Or are you talking the web version use of
state.videoPlayerTemporaryHeight. It's really just naming. I've simplified the logic involved in callingsetVideoPlayerHeightvs the web versionsetVideoPlayerTemporaryHeightbut the intent is the same. I expect the calling logic to become more complex in future changes so I opted to keep it simple for now.
I found the use of temporary misleading in the web version. I couldn't find a condition where the "temporary" height is not used, it is ignored once the embed is rendered but it's not replaced or removed. Perhaps a better name, in both cases, isplayerWrapperHeightsince that is where how the state property is used.
Based on the following comment, I understand that the temporary height is just an initial calculation we make based on the block's width and the video's aspect ratio. Once the player is loaded, the height is replaced with the final value, so it seems the temporariness is based on the fact that is only used during the loading state.
There was a problem hiding this comment.
It's true that there is an initial value for the height that is derived from the aspect ratio.
The issue I see is the naming. From what I observed the videoPlayerTemporaryHeight state variable is always used to set the style height property.
In fact once the video is available, videoPlayerTemporaryHeight is set to auto here
In other words, this state variable has an initial value to set a placeholder height, and then is set to auto once the actual height is available. To me that variable is not temporary. If I see something named temporary with a comment that it's eventually ignored, I would expect to see it used like
const value = actualValue || temporaryValue
instead the code here is more like
let temporaryValue = 1;
eventually( (actualValue) => setTemporaryValue(actualValue) )
const value = temporaryValue
There was a problem hiding this comment.
In fact once the video is available,
videoPlayerTemporaryHeightis set toautohereIn other words, this state variable has an initial value to set a placeholder height, and then is set to
autoonce the actual height is available. To me that variable is not temporary. If I see something named temporary with a comment that it's eventually ignored, I would expect to see it used like
Yep, good point. I agree the videoPlayerTemporaryHeight is a bit confusing, especially when the auto value is set because is not consistent with the temporariness led by the variable name. I understand that the purpose of auto is basically disabling the temporary height and letting the video player control the height, maybe the value should be renamed to disabled or something similar 🤔. For the native version, if we have to provide a customized version of the component, we can discuss further how to name and structure this part if needed.
...videopress/src/client/block-editor/blocks/video/components/videopress-player/index.native.js
Outdated
Show resolved
Hide resolved
* cli: `generate` command should ask for plugin versioning method We've been defaulting to semver like we do for all other project types, but it seems this may be confusing teams working on the plugins who may be expecting WordPress-style versioning. Instead let's ask when the plugin is being created.
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
* Add a pricing-utils helper folder This folder will store helper functions related to pricing. For now, I've only included `isFirstMonthTrial` as it's the only one required for this PR, but intention is to add more functions related to pricing, instead of having them only as reducers/selectors. * Recommendations: remove discount if there's a trial * Recommendations: remove the coupon code from the URL if there's a trial This change is made in the component instead of in withUpgradeUrl because this rule is more strict and related specifically to what is displayed in this card. This change is mostly to avoid visual confusion. * Add changelogger * Remove unnecessary CSS * Fix changelogger validity
…us (#28820) * Increase form fields padding based on user-defined border-radius * changelog * Fix field padding in the Forms package * changelog * Fix changelogger
* Odyssey: Remove Calypso nudge. Doesn’t make sense to push to Calypso now that Odyssey is an option. * Odyssey: Remove Calypso nudge (2). This time from the loading UI. * Changelog++
* [not verified] Add block support for font family in Related Posts block server block renderer * [not verified] changelog
…ideo (#29183) * remove image for poster when uploading video * fix default aspect ratio * changelog
* Jetpack: update to-test.md for v11.9 * Typos and formatting
…#29198) * Update the forms dashboard to use the same feedback menu item * changelog
github-actions
bot
added
[Package] Search
Fixes wordpress-mobile/gutenberg-mobile#5509
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
TBD