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 ScrollView to be controllable via scrollTop #740

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joefiorini
Copy link

Allows setting/syncing scrollTop property of ScrollView so consumers can dynamically update scroll position as needed. Adds onScroll prop to allow the parent component to keep the scroll value in sync when the user manually scrolls.

@joefiorini
Copy link
Author

@Et7f3 looks like the build failed due to cache restore failures on macOS and Linux. Then the test run hung on macOS with no output (and timed out after 15 minutes). Could this be a build agent issue? Is it possible to rerun the checks?

Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

We've done something similar for the TreeVIew in Oni2, except it uses a spring animation instead of a custom bounce mechanism, and supports alignment using other reference poiits than just top. But maybe it can serve as a source of inspiration anyway.

Most of the complexity of the scrolling mechanism is encapsulated in a custom useScroll hook, which is then used here.

Comment on lines +60 to +72
let%hook () =
Hooks.effect(
Always,
() => {
if (scrollTop != actualScrollTop) {
switch (onScroll) {
| Some(_) => dispatch(ScrollUpdated(scrollTop))
| None => ()
};
};
None;
},
);
Copy link
Member

@glennsl glennsl Jan 30, 2020

Choose a reason for hiding this comment

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

I don't think this will work if ~scrollTop is omitted, since it would then always reset to 0, its default value. So either scrollTop and onScroll needs to be mandatory, which I don't think is desirable, or scrollTop needs to be optional but without a default value so this can be skipped if it's not set.

Copy link
Author

Choose a reason for hiding this comment

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

@glennsl I just tested it by adding an additional ScrollView to the example and leaving off the onScroll and scrollTop. Everything seemed to work fine in that case. Then I passed just a scrollTop and it rendered as expected. Are you referring to a case where scrollTop is missing but onScroll is provided?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah that's a necessary condition too. It should check for the presence of scrollTop instead of onScroll. It's not very intuitive that hard-coding scrollTop and not setting onScroll would result in scrollTop being ignored.

I also don't think the effect hook is really necessary, and mostly just complicates the logic. Instead you could rename actualScrollTop to something like InternalScrollTop and then set actualScrollTop to scrollTop if present, or internalScrollTop if not.

@Et7f3
Copy link
Member

Et7f3 commented Jan 30, 2020

@joefiorini I had rerun the check and it is 👌.

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.

3 participants