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

Allow overriding style on web #4333

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

M-i-k-e-l
Copy link

Summary

Allow overriding style on web

Motivation

When overriding the style with {} we can allow the (web) video to resize to its real size without further effort.

Changes

Sending style will now affect video on web.

Test plan

I assume that if someone is already sending style this can affect them, however the release of the web support was quite recent so I'm not sure how risky this is; not sure how to test it though...

@moskalakamil
Copy link
Member

I'm just wondering- if you have a react-native app you have one

@KrzysztofMoch KrzysztofMoch self-requested a review December 14, 2024 11:55
@M-i-k-e-l
Copy link
Author

I'm just wondering- if you have a react-native app you have one

If you're referring to the testing, then I've tested it; I assumed the the Test plan is for unit test of some sort

@moskalakamil
Copy link
Member

Sorry, I'm not sure why my comment was cut off. What I meant was: if you have a Video component in your app that works for both web and native, do the types work correctly? Do they change, for example, when you use Platform.select? Also, what is the default type in that case?

@M-i-k-e-l
Copy link
Author

Sorry, I'm not sure why my comment was cut off. What I meant was: if you have a Video component in your app that works for both web and native, do the types work correctly? Do they change, for example, when you use Platform.select? Also, what is the default type in that case?

If this is about the typing change, then I did not test that, I needed to send an empty style {}...
I made the change to the type because of the error in this project, not because of a usage issue - perhaps the correct thing was to ignore it.

style?: CSSProperties;
}

export type WebReactVideoProps = Omit<ReactVideoProps, 'style'> & WebStyleProp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't you patch directly ReactVideoProps instead of creating WebStyleProp in this common file ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

something like:

type CustomViewProps = Omit<ViewProps, 'style'> & {
  style?: ViewProps['style'] | CSSProperties;
};

export interface ReactVideoProps extends ReactVideoEvents, CustomViewProps {

@freeboub
Copy link
Collaborator

@zoriya
Copy link
Contributor

zoriya commented Dec 21, 2024

I purposely omitted the style props because this is not compatible with react native styles. this means you'll need a different style for web & native. Since types are different, Platform.select is bothersome to use & styling libs would just error.
Existing projects using the style property for native would probably have a type error due to the new style typing.

Maybe we could add a webStyle instead to prevent those issues.

@M-i-k-e-l
Copy link
Author

Another option is to add a prop that will have hug (style={{}}) and fill (style={videoStyle}) as values.

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.

4 participants