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(android): Change subtitleLayout from child to sibling of layout #3830

Merged

Conversation

YangJonghun
Copy link
Collaborator

@YangJonghun YangJonghun commented May 23, 2024

Summary

Change subtitleLayout from child to sibling of layout

Motivation

I think the behavior of subtitle position changing by resize mode is a bug, but if not, please close it.

Changes

  • as-is
  • to-be

Test plan

change resize mode (CONTAIN / COVER)

@freeboub
Copy link
Collaborator

@YangJonghun I think you will fix this issue: #3662
Can you please confirm, thank you !

@YangJonghun
Copy link
Collaborator Author

@freeboub
Maybe..? I think what he really wanted was a custom style

@freeboub
Copy link
Collaborator

@freeboub Maybe..? I think what he really wanted was a custom style

Not sure, let's ask in the ticket !

@freeboub freeboub merged commit c2cc917 into TheWidlarzGroup:master May 28, 2024
3 checks passed
@YangJonghun YangJonghun deleted the fix/android-subtitle-position branch May 28, 2024 12:15
@freeboub
Copy link
Collaborator

@YangJonghun This patch cause following regression: #4019
I am not sure on how to fix it ...
Do you think we should add a prop:

subtitleFollowVideoView
-> true : old video style -> The subtitle will always match video position
-> false: your video style -> The subtitle will always match main view position

Maybe a string prop is better for flexibility in the futur also :/

Because depending of app expectation we can choose between the 2 options ?

@freeboub
Copy link
Collaborator

freeboub commented Sep 2, 2024

@YangJonghun
Please see last merged PR. The proposed change was causing a regression, the original behavior has been restored, so in next release you will have to add a prop to configure it.

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.

2 participants