Skip to content

Fix ScrollView onScroll event only being called once.#452

Closed
chrene wants to merge 3 commits into
react:masterfrom
chrene:scrollview-event
Closed

Fix ScrollView onScroll event only being called once.#452
chrene wants to merge 3 commits into
react:masterfrom
chrene:scrollview-event

Conversation

@chrene

@chrene chrene commented Mar 29, 2015

Copy link
Copy Markdown

No description provided.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@tadeuzagallo

Copy link
Copy Markdown
Contributor

@nicklockwood I think you wrote the TODO, is that right?
I don't think 0 should be the default value...

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sahrens

sahrens commented Mar 29, 2015

Copy link
Copy Markdown
Contributor

We don't want to send the events by default, and we want the client to think about what frequency they need the events. If the parent specifies onScroll and not the frequency, perhaps we should redbox?

On Mar 29, 2015, at 12:00 PM, Tadeu Zagallo notifications@github.com wrote:

@nicklockwood I think you wrote the TODO, is that right?
I don't think 0 should be the default value...


Reply to this email directly or view it on GitHub.

@tadeuzagallo

Copy link
Copy Markdown
Contributor

@sahrens it might have been my fault, I was talking @chrene on IRC and he said it was a bug, and I totally forgot that we needed to set the property for it to work.
If we set the property is working as expected? Because if it is maybe we should remove that comment.

@chrene

chrene commented Mar 29, 2015

Copy link
Copy Markdown
Author

The only real bug was just that it was a double when it should have been a NSUInteger. Since it was a double it got a very very high number when set. Like 4 billion or something. Now it gets the real value. We can keep the check in the if statement such that the client needs to specify another value than 0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: this indentation was right...

@tadeuzagallo

Copy link
Copy Markdown
Contributor

I tested it and it's working as expected, but @sahrens I tried adding an invariant instead of the current warning, and during testing it on UIExplorer I just noticed that an invariant during a NavigatorIOS push crashes the objC... Any idea about that? Should we add the invariant anyways?

@sahrens

sahrens commented Mar 30, 2015

Copy link
Copy Markdown
Contributor

If there is already a warning we can leave it.

On Mar 29, 2015, at 5:20 PM, Tadeu Zagallo notifications@github.com wrote:

I tested it and it's working as expected, but @sahrens I tried adding an invariant instead of the current warning, and during testing it on UIExplorer I just noticed that an invariant during a NavigatorIOS push crashes the objC... Any idea about that? Should we add the invariant anyways?


Reply to this email directly or view it on GitHub.

@nicklockwood

Copy link
Copy Markdown
Contributor

I submitted a fix for this today that changes the property type to NSTimeInterval and renames the property to scrollEventThrottle, which seems more logical. That also fixes the issue with the default value being set incorrectly.

@sahrens

sahrens commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

This should be fixed in master - please open an issue if still see the bug.

@sahrens sahrens closed this Apr 1, 2015
facebook-github-bot pushed a commit that referenced this pull request Mar 9, 2017
Summary:
This adds some improvements to the new ```YGConfig```, it tackles #452 and moves the scalefactor into the config.
Closes react/yoga#457

Differential Revision: D4675088

Pulled By: emilsjolander

fbshipit-source-id: 99b2c734d6c5139fe1dc8bdeb014bb038f0e337d
jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
mganandraj added a commit to mganandraj/react-native that referenced this pull request Jun 16, 2020
…h latest Android toolchain (react#452)

* Switching the build and publish pipeline to Ubuntu-18.04 machines as they tends to have newer build pipelines.

* Using node v12.x
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.

5 participants