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

ScrollView properties/events missing #311

Closed
SergejDK opened this issue Feb 3, 2019 · 9 comments
Closed

ScrollView properties/events missing #311

SergejDK opened this issue Feb 3, 2019 · 9 comments
Labels
t/enhancement New feature or request

Comments

@SergejDK
Copy link
Collaborator

SergejDK commented Feb 3, 2019

I think we currently can't get and set the current scrollposition of the scrollview.

If I am right these parts should be inclueded:
Properties

  • ScrollX
  • ScrollY

I don't know if other events should be included like e.g. SetScrolledPosition. When we have the scrollX and scrollY property the events should not be needed.

@TimLariviere
Copy link
Member

TimLariviere commented Feb 4, 2019

@SergejDK Good find. We're indeed missing a bunch of properties and events on ScrollView.

In addition to ScrollX and ScrollY, we need to add the events Scrolled and ScrollToRequested.

I'm not sure how to correctly support the method ScrollToAsync though.
Should we let people get a reference to the control themselves to call ScrollToAsync, or should we transform it into a parameter of ViewElement?

E.g.

let scrollViewRef = ViewRef<ScrollView>()
View.ScrollView(ref=scrollViewRef)

// Some time later
async {
    do! scrollViewRef.ScrollToAsync(10.0, 20.0, true)  
}

OR

View.ScrollView(scrollTo=(10.0, 20.0, true))

The 2nd one looks cleaner. But we won't be able to await it...

@SergejDK
Copy link
Collaborator Author

SergejDK commented Feb 4, 2019

Is one way of the examples for ScrollToAsync easier to test?

@TimLariviere
Copy link
Member

TimLariviere commented Feb 4, 2019

Correction: ScrollX and ScrollY are read-only. Only the events Scrolled and ScrollToRequested should be added to Fabulous.

Is one way of the examples for ScrollToAsync easier to test?

You mean for unit tests?
If so, the 2nd one is also the easiest to unit test. It's only a tuple value.

@SergejDK
Copy link
Collaborator Author

SergejDK commented Feb 4, 2019

What is the difference between the AcceleratorProperty of the MenuItem and the ScrollX/Y? They are both readonly.

Yes I meant for unit tests. Well then I think the 2nd solution is okay. Or is there some other profit with using the 1st solution ?

@TimLariviere
Copy link
Member

TimLariviere commented Feb 4, 2019

Accelerator is a dependency property with static getter/setter. (GetAccelerator/SetAccelerator)

ScrollX/ScrollY are standard properties that are readonly. They have their corresponding backing dependency properties (ScrollXProperty) but lack a way to set them, other than ScrollToAsync. (SetScrolledPosition is marked for internal use only)

And when writing views, we only create new ViewElements to describe how we want the view to be.
We can't read the current values of the controls on the UI, other than though ViewRef/Created.
So these properties are useless for Fabulous.

So from an out-of-XF viewpoint, ScrollX and ScrollY are readonly, Accelerator is not.
That's why you were able to add support for Accelerator.

@TimLariviere
Copy link
Member

Yes I meant for unit tests. Well then I think the 2nd solution is okay. Or is there some other profit with using the 1st solution ?

With the first solution, the only 2 advantages I see are :

  • Ability to await the scroll animation to finish
  • Be closer to the way Xamarin.Forms works (so less docs to write and easier to move to new XF versions)

But the drawback is it not really the way Fabulous works.
The 2nd one is better in that way.

@SergejDK
Copy link
Collaborator Author

SergejDK commented Feb 4, 2019

Accelerator is a dependency property with static getter/setter. (GetAccelerator/SetAccelerator)

ScrollX/ScrollY are standard properties that are readonly. They have their corresponding backing dependency properties (ScrollXProperty) but lack a way to set them, other than ScrollToAsync. (SetScrolledPosition is marked for internal use only)

And when writing views, we only create new ViewElements to describe how we want the view to be.
We can't read the current values of the controls on the UI, other than though ViewRef/Created.
So these properties are useless for Fabulous.

So from an out-of-XF viewpoint, ScrollX and ScrollY are readonly, Accelerator is not.
That's why you were able to add support for Accelerator.

Great explanation! 👍
Thank you :)

@SergejDK
Copy link
Collaborator Author

SergejDK commented Feb 4, 2019

Yes I meant for unit tests. Well then I think the 2nd solution is okay. Or is there some other profit with using the 1st solution ?

With the first solution, the only 2 advantages I see are :

* Ability to await the scroll animation to finish

* Be closer to the way Xamarin.Forms works (so less docs to write and easier to move to new XF versions)

But the drawback is it not really the way Fabulous works.
The 2nd one is better in that way.

I do think that we can go with the 2nd one. Normally there shouldn't be a problem when we move to a new XF version or are the some special part to be aware of. The documentation part is okay to write, I mean it needs to be written either way. In some cases there are already some differences to XF so I think it is better to stay with the Fabulous way.

@TimLariviere
Copy link
Member

The documentation part is okay to write, I mean it needs to be written either way.

Ideally yes, but if we can just point people to docs.microsoft.com and not write the docs ourselves, it will save us a lot of work. :)

I do think that we can go with the 2nd one.

Ok, I'm good with it.
We will need a documentation explaining how to have a similar "awaitable" experience the Fabulous way (by using the Scrolled event I guess? -- I don't mean using async/let!).
And I think it will be good to also add the Xamarin.Forms way to the docs, to let people know they can still use the classic ScrollToAsync if they want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants