-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
Update to XF 3.4 complete. #257
Conversation
There's also the |
@slang25 Thanks that is what I wanted to know. Added the file. |
Looks like this also needs to change: and also the template package version: |
@slang25 Thanks! Found a another place with hard coded string versions. Any reason we can't pull this from paket with the build script? |
I think the wrappers also need to be updated with this Here's an API diff. Not too many on existing controls and then there's a new ImageButton control |
@PureWeen Thanks! I has thinking some of that might show up as a compiler error. I will look at the wrappers. |
@rbrogan-git Sorry, should have been more explicit in the issue. Just bumping the version number is not enough. Fabulous requires a minimum version of Xamarin Forms (currently 3.1) but devs are free to update on their own. What really interests us here is, like @PureWeen said, to update https://github.com/fsprojects/Fabulous/blob/master/tools/Generator/Xamarin.Forms.Core.json with the wrappers around the new controls, the new properties, the changed features, etc. between Xamarin.Forms 3.1 and 3.4. You can take a look at a previous PR which updated from 3.0 to 3.1 : #178 |
Could this be a 2 step process? Update the dependencies fixing any breaking changes, then PRs to lift through new changes. Or does that risk missing some of the apis? I presume the json was semi-automated and now a manual task to add to? |
The json file is fully manual for now (which causes some issues like forgotten controls or properties). I don’t feel like XF 3.4 is a major enough update to require a 2 step process. Like you said, it would be hard to track every new features if we do it in separate PRs. Also people might expect full support if we require them to update to XF 3.4. |
@TimLariviere I understand the ask. I reviewed the pull request to see what was changed from 3.0 to 3.1. |
@rbrogan-git Unfortunately no, we don't have that kind of documentation... The idea here is to add/update/remove any control/property/event accordingly in the (Tip: You can run the generator quickly with the command The way the JSON file is defined is as such:
To enable more customization, there's a bunch of other attributes.
If we take an example: {
"name": "Xamarin.Forms.BoxView",
"members": [
{
"name": "Color",
"defaultValue": "Xamarin.Forms.Color.Default"
},
{
"name": "CornerRadius",
"defaultValue": "Unchecked.defaultof<Xamarin.Forms.CornerRadius>"
}
]
}, I used Another example: {
"name": "Xamarin.Forms.WebView",
"members": [
{
"name": "Source",
"uniqueName": "WebSource",
"defaultValue": "null"
},
{
"name": "Navigated",
"defaultValue": "null",
"inputType": "Xamarin.Forms.WebNavigatedEventArgs -> unit",
"convToModel": "(fun f -> System.EventHandler<Xamarin.Forms.WebNavigatedEventArgs>(fun _sender args -> f args))"
},
{
"name": "Navigating",
"defaultValue": "null",
"inputType": "Xamarin.Forms.WebNavigatingEventArgs -> unit",
"convToModel": "(fun f -> System.EventHandler<Xamarin.Forms.WebNavigatingEventArgs>(fun _sender args -> f args))"
},
{
"name": "ReloadRequested",
"defaultValue": "null",
"inputType": "System.EventArgs -> unit",
"convToModel": "(fun f -> System.EventHandler(fun _sender args -> f args))"
}
]
}, I hope it's clear. |
@TimLariviere yes this is immensely helpful! Just what I needed to see. Glad the x.f.core.fs is generated. Makes much more sense now. |
@TimLariviere This is my first pass on fixing up the json file. I tried to add the TitleView property but was not successful so I kept it out for now. I am unclear on how to setup the convert function for View. Suggestions welcome. Let me know if I missed anything. |
@TimLariviere I know I am missing the TitleView property but other than that I think I got all the additions. Thanks and Happy New Year! |
@rbrogan-git Thanks! I'll take a look soon. Could you rebase your PR on the last commit on |
@TimLariviere I wondered why the diff got so weird. Updated. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! A lot of the new features are covered.
In additions to my review comments, here is a list of missing properties/controls that I found.
- Button
Padding (Thickness)It is already here, I missed it
- ImageButton
- Command (unit -> unit) -- Same as
Command
on Button IsLoading (bool) -- Settable via this.SetIsLoading(bool)IsPressed (bool) -- Settable via this.SetIsPressed(bool)
Finally, not sure it's a good idea to add IsLoading/IsPressed as their setter methods are markedEditorBrowsableState.Never
- Command (unit -> unit) -- Same as
- NavigationPage
- TitleView -- Settable via NavigationPage.SetTitleView(this, view)
- SwipeGestureRecognizer
- Command (unit -> unit) -- Same as
Command
on Button - Direction (SwipeDirection / default
enum<SwipeDirection>(0)
) - Threshold (uint32 / default 100)
- Event Swiped (EventHandler
<SwipedEventArgs
>)
- Command (unit -> unit) -- Same as
- WebView
- Reload (bool) -- Not an XF property (it's a method: myWebView.Reload()), but I think we need to expose it as a property like
invalidate
of SkiaSharp
https://github.com/fsprojects/Fabulous/blob/97417123e046cc3738f78d3469e638e3c47e8c19/extensions/SkiaSharp/SkiaSharp.fs#L63
- Reload (bool) -- Not an XF property (it's a method: myWebView.Reload()), but I think we need to expose it as a property like
For TitleView, I still need to find the correct way to do it. |
Oops, sorry for the conflict. |
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
Co-Authored-By: rbrogan-git <[email protected]>
@rbrogan-git Here's a way to implement In the json file, we declare the property like usual but Xamarin.Forms is expecting a {
"name": "TitleView",
"defaultValue": "null",
"inputType": "ViewElement",
"updateCode": "updateNavigationPageTitleView"
}, We then need to add this update function into https://github.com/fsprojects/Fabulous/blob/master/src/Fabulous.Core/ViewConverters.fs let updateNavigationPageTitleView (prevOpt: ViewElement voption) (currOpt: ViewElement voption) (target: Xamarin.Forms.NavigationPage) =
match prevOpt, currOpt with
| ValueSome prev, ValueSome curr when identical prev curr -> ()
| ValueSome prev, ValueSome curr when canReuseChild prev curr ->
updateChild prev curr (NavigationPage.GetTitleView(target))
| _, ValueSome curr ->
NavigationPage.SetTitleView(target, (curr.Create() :?> Xamarin.Forms.View))
| ValueSome _, ValueNone ->
NavigationPage.SetTitleView(target, null)
| _, _ -> () Might be some errors in this code. I haven't tested it yet |
@TimLariviere thanks for all the information and help with this pull request. I learned a lot about this project. Sorry you ended up finishing it yourself. I hope the work I did was helpful. |
@rbrogan-git Your work was more than helpful. You did the most complex and time-consuming part by adding the new properties. :) |
Issue #243
I am not an expert in paket but I think this is all that is needed, correct? I ran the builds and tests after this update successfully. I also launched the tic tack toe and all controls samples for android and all is working fine.
Let me know if this is good or of you need anything else.
Thanks!
-Roger