search

LunatiqueCoder / react-native-media-console

A React Native video player. Built with TypeScript ❤️
MIT License
194 stars 29 forks source link

[+] Replace disableSeekbar with disableSeekBar #75

Closed esegebart closed 1 year ago

esegebart commented 1 year ago

[+] Disable seek bar was not working

VideoPlayerProps were being passed as disableSeekBar to the the variable disableSeekbar and was not disabling it properly. Changed the prop to capitalize the B everywhere and in documentation and now all is working :) Also tested and it disables it properly now.

esegebart commented 1 year ago

@esegebart Did I need to make an issue ticket for this? I guess I don't know the protocol if I find a bug :)

LunatiqueCoder commented 1 year ago

@esegebart It's nice to have, but not a requirement. We're the only active maintainers so we can decide together.

Sorry for the summer time being quite inactive. I'll try to have a better look this weekend or the next 🙈

esegebart commented 1 year ago

@LunatiqueCoder You do not need to apologize :) Glad you are enjoying your summer. Like you said, it's open source, there is no rush 😎

LunatiqueCoder commented 1 year ago

Not sure what's the issue here, but disableSeekbar seems to be working for me? I'm afraid to change the naming because it will introduce a little breaking change

What else can you tell me about this issue? 🤔

esegebart commented 1 year ago

Oh! It was not working on Android. I couldn't disable the seekbar. When the object was returned - it returned disableSeekBar and the variable it was trying to be set to was disableSeekbar. So it was always false and never got set true. So I changed them all to disableSeekBar in the project to try and avoid any breaking changes. :( but I guess it broke it still?

I was trying to disable the seekbar for a live stream and this was only on android. It remained false and tracing it lead me to the object that was returned having a mismatch in naming for what was trying to accept it.

On Thu, Sep 21, 2023, 2:18 PM Ovidiu Cristescu @.***> wrote:

Not sure what's the issue here, but disableSeekbar seems to be working for me? I'm afraid to change the naming because it will introduce a little breaking change

What else can you tell me about this issue? 🤔

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730160460, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFFIMWY7LD57M4I4X2TX3SHIZANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

LunatiqueCoder commented 1 year ago

@esegebart Oh, ok then, I think it's better to change them all to `disableSeekbar' - this way it will not introduce breaking changes for the users. Do you want me to do this change or perhaps you want to improve your contribution stats on the repository? 😂

esegebart commented 1 year ago

I think that is what I thought I did in this pull request is replace them all? Is that not true?

On Thu, Sep 21, 2023, 2:32 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart Oh, ok then, I think it's better to change them all to `disableSeekbar' - this way it will not introduce breaking changes for the users. Do you want me to do this change or perhaps you want to improve your contribution stats on the repository? 😂

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730177359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFGCZT7TDSFC3PV5WN3X3SI4FANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

esegebart commented 1 year ago

Oh! Ok you want them to be the other way. I will send over here in 10 min!

On Thu, Sep 21, 2023, 2:32 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart Oh, ok then, I think it's better to change them all to `disableSeekbar' - this way it will not introduce breaking changes for the users. Do you want me to do this change or perhaps you want to improve your contribution stats on the repository? 😂

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730177359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFGCZT7TDSFC3PV5WN3X3SI4FANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

esegebart commented 1 year ago

@LunatiqueCoder Wait no bc then that will make it break again for Android. The Android object returned has disableSeekBar - and disableSeekbar in the code did not work. So I'd be putting it back to the way it was when it was broken

LunatiqueCoder commented 1 year ago

@esegebart hmm, this is weird, i'm wondering where is this Android object coming from? 🤔 Is it from the props?

LunatiqueCoder commented 1 year ago

@esegebart It seems to me that you might be setting disableSeekBar yourself from a parent component. I couldn't find any inconsistency in this project 🙈 disableSeekBar doesn't exist at all in the project.

esegebart commented 1 year ago

@LunatiqueCoder Yes in the VideoPlayerProps

LunatiqueCoder commented 1 year ago

@esegebart I think we should jump in google meet, perhaps with a shared screen we can also share ideas easier?

esegebart commented 1 year ago

I'll have to look at the comparisons again to verify what I'm saying, I'm headed back to work now and I'll take a look. Maybe it was!

On Thu, Sep 21, 2023, 2:44 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart It seems to me that you might be setting disableSeekBar yourself from a parent component. I couldn't find any inconsistency in this project 🙈 disableSeekBar doesn't exist at all in the project.

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730195543, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFCYMVQLQQM65BKIMUTX3SKKLANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

LunatiqueCoder commented 1 year ago

@esegebart Don't worry, no pressure! I'll start being more active now :)

esegebart commented 1 year ago

Well, I know I'm not crazy. I hadn't changed anything and I had to hunt down why it was not setting the disableSeekbar to true. It remained false. I'm trying to remember the object I was console logging. Will let you know.

On Thu, Sep 21, 2023, 2:44 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart It seems to me that you might be setting disableSeekBar yourself from a parent component. I couldn't find any inconsistency in this project 🙈 disableSeekBar doesn't exist at all in the project.

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730195543, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFCYMVQLQQM65BKIMUTX3SKKLANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

LunatiqueCoder commented 1 year ago

@esegebart You're not using TypeScript on your project right?

esegebart commented 1 year ago

No, I am not :(

On Thu, Sep 21, 2023, 4:11 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart You're not using TypeScript on your project right?

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730308239, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFCKYN4AMVIRMHIS453X3SURBANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

LunatiqueCoder commented 1 year ago

@esegebart Then you are not crazy, but you are not using TypeScript either haha. This is one of those things that TypeScript does, it makes you write more code, but it enforces you to use it better and with less bugs 😌

If you can confirm everything works well for you if you use disableSeekbar everywhere, I suppose we can close this?

esegebart commented 1 year ago

Yes I will have more time to confirm this tomorrow and will message you!

On Thu, Sep 21, 2023, 4:19 PM Ovidiu Cristescu @.***> wrote:

@esegebart https://github.com/esegebart Then you are not crazy, but you are not using TypeScript either haha. This is one of those things that TypeScript does, it makes you write more code, but it enforces you to use it better and with less bugs 😌

If you can confirm everything works well for you if you use disableSeekbar everywhere, I suppose we can close this?

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#issuecomment-1730316257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFD7KTURLTSAWM3PSGTX3SVNDANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: @.***>

LunatiqueCoder commented 1 year ago

@esegebart Any news with this one? Does disableSeekbar work for you?

LunatiqueCoder commented 1 year ago

I'll close this one for now, will reopen if needed. Trying to clean up a little bit.

esegebart commented 1 year ago

So sorry! I got very busy. :( I believe you if you say you did not find anything with it. I may have just overlooked something or it was being set myself, I am not sure. When I get some time to circle back with it, I will let you know :)

On Sun, Sep 24, 2023 at 2:06 PM Ovidiu Cristescu @.***> wrote:

Closed #75 https://github.com/LunatiqueCoder/react-native-media-console/pull/75.

— Reply to this email directly, view it on GitHub https://github.com/LunatiqueCoder/react-native-media-console/pull/75#event-10455783882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVRSFGWIT4FLLUXRXZKTOLX4CAC3ANCNFSM6AAAAAA3URLLMQ . You are receiving this because you were mentioned.Message ID: <LunatiqueCoder/react-native-media-console/pull/75/issue_event/10455783882 @github.com>