Skip to content
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

feat: add ability to define poster props as Image type and render poster as custom component #3972

Merged
merged 15 commits into from
Jul 22, 2024

Conversation

moskalakamil
Copy link
Member

@moskalakamil moskalakamil commented Jul 4, 2024

Summary

I added the ability for the user to fully manage the poster, including: managing all props, rendering their own poster, and fixed a bug with displaying the poster

Closes #3013

Motivation

Issue

Changes

  • Added posterProps to allow user manage all of the poster props (incl. source, resizeMode and all others)
  • Added renderPoster props, to allow user render poster as a component
  • Update docs
  • Fixed bug -> When the video wasn't fully loaded but its image was visible, both the video and the poster were simultaneously visible.
Screenshot 2024-07-04 at 10 55 44

Test plan

Run basic example ;)

Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but let's make it so we don't introduce breaking changes

docs/pages/component/props.mdx Outdated Show resolved Hide resolved
src/types/video.ts Show resolved Hide resolved
@moskalakamil moskalakamil marked this pull request as draft July 4, 2024 14:37
@moskalakamil
Copy link
Member Author

@KrzysztofMoch Thanks for the review. It should look ok now :))

@moskalakamil moskalakamil marked this pull request as ready for review July 4, 2024 15:26
docs/pages/component/props.mdx Outdated Show resolved Hide resolved
src/Video.tsx Outdated Show resolved Hide resolved
src/Video.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@freeboub freeboub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments, but this is a good feature ! Thank you !

@moskalakamil
Copy link
Member Author

few comments, but this is a good feature ! Thank you !

Thanks for review! I'll refactor the code

@seyedmostafahasani
Copy link
Collaborator

seyedmostafahasani commented Jul 8, 2024

@moskalakamil please update the type of poster according to your changes in the example app.

@moskalakamil
Copy link
Member Author

@moskalakamil please update the type of poster according to your changes in the example app.

Hi @seyedmostafahasani! I'm not sure what do you mean, can u explain more? I added new type for poster props in example app, and i'll change to renderLoader as freeboub said

src/Video.tsx Outdated Show resolved Hide resolved
@seyedmostafahasani
Copy link
Collaborator

@moskalakamil please update the type of poster according to your changes in the example app.

Hi @seyedmostafahasani! I'm not sure what do you mean, can u explain more? I added new type for poster props in example app, and i'll change to renderLoader as freeboub said

Hey, I was suggestion to update type of poster variable, but I noticed you replaced it with a new variable.

docs/pages/component/props.mdx Outdated Show resolved Hide resolved
src/Video.tsx Show resolved Hide resolved
src/Video.tsx Outdated Show resolved Hide resolved
@moskalakamil moskalakamil changed the title feat: add ability to define poster props and render poster as custom component feat: add ability to define poster props as Image type and render poster as custom component Jul 11, 2024
@freeboub
Copy link
Collaborator

code changes looks good to me

@moskalakamil moskalakamil force-pushed the poster branch 3 times, most recently from 224ee59 to cddb5b5 Compare July 17, 2024 11:03
@moskalakamil
Copy link
Member Author

@freeboub @KrzysztofMoch conflicts resolved 👍

Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅ but there are conflicts again 😅

@moskalakamil
Copy link
Member Author

We need to be faster 😅 I resolved them @KrzysztofMoch

@KrzysztofMoch
Copy link
Member

Waiting for green CI and I will merge

@KrzysztofMoch KrzysztofMoch merged commit adbd06e into TheWidlarzGroup:master Jul 22, 2024
3 checks passed
@moskalakamil
Copy link
Member Author

Thanks 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set header or custom component for poster
4 participants