deanhet / react-native-text-ticker

React Native Text Ticker/Marquee Component
MIT License
444 stars 75 forks source link

import ScrollView from gesture-handler #112

Closed ManoelPradoMark22 closed 2 years ago

ManoelPradoMark22 commented 2 years ago

IMPORTANT!!! import ScrollView from react-native-gesture-handler instead just react-native. this mode allows the user - even if the TextTicker is already inside a ScrollView (horizontal) - to grab the text and drag (with the 'scroll' property active). Without the gesture-handler, this is not possible, the drag does not work (only scroll the FATHER ScrollView, the son doesn't scroll). Video showing the problem and after fixed: https://www.youtube.com/watch?v=Ev1_tT98kDI&ab_channel=ManoeldeOliveiraPradoNeto Project as an example in the following repository: https://github.com/ManoelPradoMark22/ignite_reactNative_gofinances_project2 My commit when I add the react-native-text-ticker lib: https://github.com/ManoelPradoMark22/ignite_reactNative_gofinances_project2/commit/0f629037a04547345e2125f0b66491c655199f60

deanhet commented 2 years ago

Thanks so much for this pull request. The video, the description. It's all very clear and helped me understand the problem you're having.

However, I think there's a better approach to this. This pull request would mean future versions of the text ticker would have a dependency on react-native-gesture-handler. For future reference, your code works because you already have gesture-handler installed. This would break for everyone else if they don't have it installed. The gesture handler would have to be declared as a dependency of this project inside its package.json.

The dependency is also a relatively large 1.34mb compared to the source code in this repo. Everyone would have to install that, even if they weren't going to use the scrolling functionality.

The problem you're having is that the parent is swallowing the touch events. By changing the scrollview to gesture-handler you're letting it correctly delegate where the touches go. A more generic approach that could work for everyone would be to pass a new prop to the textTicker, onScrollTouch that will have a callback of true or false. If you look in the source code, you could call this prop in the scrollBegin and scrollEnd functions.

The boolean it returns would drive the scrollEnabled prop of your parent scrollview. So when you start to drag the text ticker, it disables scrolling for the parent. When you let go, it gives control back to the parent.

Feel free to test and implement the above and submit a PR if you like, alternatively you could just keep using your branch as it works well for your use case.

Again, thanks for taking the time to make such a thorough description.

ManoelPradoMark22 commented 2 years ago

Thanks so much for this pull request. The video, the description. It's all very clear and helped me understand the problem you're having.

However, I think there's a better approach to this. This pull request would mean future versions of the text ticker would have a dependency on react-native-gesture-handler. For future reference, your code works because you already have gesture-handler installed. This would break for everyone else if they don't have it installed. The gesture handler would have to be declared as a dependency of this project inside its package.json.

The dependency is also a relatively large 1.34mb compared to the source code in this repo. Everyone would have to install that, even if they weren't going to use the scrolling functionality.

The problem you're having is that the parent is swallowing the touch events. By changing the scrollview to gesture-handler you're letting it correctly delegate where the touches go. A more generic approach that could work for everyone would be to pass a new prop to the textTicker, onScrollTouch that will have a callback of true or false. If you look in the source code, you could call this prop in the scrollBegin and scrollEnd functions.

The boolean it returns would drive the scrollEnabled prop of your parent scrollview. So when you start to drag the text ticker, it disables scrolling for the parent. When you let go, it gives control back to the parent.

Feel free to test and implement the above and submit a PR if you like, alternatively you could just keep using your branch as it works well for your use case.

Again, thanks for taking the time to make such a thorough description.

I understood perfectly, and you are absolutely right. I found the modification idea you suggested brilliant! And I tested but it still doesn't work. Because for the SON's props scrollBegin and scrollEnd to work, the FATHER's scrollEnabled must be set to false. Once the FATHER's scrollEnabled changes to true the SON's scrollBegin and scrollEnd properties stop responding.

You said: "alternatively you could just keep using your branch as it works well for your use case." Would this form only be possible locally? If I want to use the library in an online project, how can I use this library with that change I made with the ScrollView from react-native-gesture-handler? Is just add the lib in my project: yarn add ManoelPradoMark22/react-native-text-ticker instead: yarn add react-native-text-ticker Adding my fork I will be abble to use your lib, but also my changes, right? My fork continues getting updates from the oficial lib? Sorry so many questions, I don't have so much experience with packages and libs

deanhet commented 2 years ago

Ah, of course. Yeah I think you'll have to use your own in this case. You can add your fork with these instructions: https://blaipratdesaba.com/how-to-use-an-npm-node-module-that-has-been-forked-b7dd522fdd08 It will be frozen at that point in time with that code though. If you would like to stay up to date with this version you'll have to merge the changes from here into yours when updates happen. Alternatively, you could keep using this package and apply your changes on top with https://www.npmjs.com/package/patch-package.

ManoelPradoMark22 commented 2 years ago

Ah, of course. Yeah I think you'll have to use your own in this case. You can add your fork with these instructions: https://blaipratdesaba.com/how-to-use-an-npm-node-module-that-has-been-forked-b7dd522fdd08 It will be frozen at that point in time with that code though. If you would like to stay up to date with this version you'll have to merge the changes from here into yours when updates happen. Alternatively, you could keep using this package and apply your changes on top with https://www.npmjs.com/package/patch-package.

Thanks so much. God bless you.