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

XF Upgrade 4.0 #416

Merged
merged 9 commits into from
May 24, 2019
Merged

XF Upgrade 4.0 #416

merged 9 commits into from
May 24, 2019

Conversation

SergejDK
Copy link
Collaborator

@SergejDK SergejDK commented May 21, 2019

fix #412
fix #417

Fuget: https://www.fuget.org/packages/Xamarin.Forms/4.0.0.425677/lib/netstandard2.0/diff/3.6.0.344457/
There are some open parts on this PR:

  • Navigationpage
    -- TitleIconImageSource is the only part:
    @TimLariviere is it an attachedproperty?

  • Searchhandler: selecteditem

  • shellnavigationstate: location

@TimLariviere
Copy link
Member

TimLariviere commented May 22, 2019

Navigationpage
-- TitleIconImageSource is the only part:
@TimLariviere is it an attachedproperty?

It is. Just like the now obsoleted TitleIcon.
https://github.com/xamarin/Xamarin.Forms/blob/3a88b01438ccedaa508a8367ead3e052a6f6e57d/Xamarin.Forms.Core/NavigationPage.cs#L29

"defaultValue": "false",
"modelType": "bool",
"inputType": "bool",
"updateCode": "updateIsFocusesd"
Copy link
Member

Choose a reason for hiding this comment

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

Little typo here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. Maybe not such a good idea to work on this in the night :D

@SergejDK
Copy link
Collaborator Author

@TimLariviere
do you think we need the ShellNavigationState?
For me it seems like the class is mostly used internally and not sth. that we should provide?
This was in the upgrade to 3.5 but I think we do not need it.

@SergejDK SergejDK marked this pull request as ready for review May 22, 2019 19:11
@SergejDK SergejDK changed the title [WIP] XF Upgrade 4.0 XF Upgrade 4.0 May 22, 2019
@TimLariviere
Copy link
Member

do you think we need the ShellNavigationState?
For me it seems like the class is mostly used internally and not sth. that we should provide?

You're right, we shouldn't provide it.
It's a read-only class built by Xamarin.Forms (only 1 get only property)
https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.shellnavigationstate.location?view=xamarin-forms

According to the documentation:

The ShellNavigationState object is constructed by the GoToAsync method, from a string, or a Uri, and it has its Location property set to the string or Uri argument.

https://docs.microsoft.com/en-us/xamarin/xamarin-forms/app-fundamentals/shell/navigation#perform-navigation

@SergejDK
Copy link
Collaborator Author

Well then I think everything should be in place and ready for merge.
I think travis does not want to work today ....

@TimLariviere
Copy link
Member

Travis doesn't seem very happy with the code (for once it's not Xamarin.Android)

/Users/travis/build/fsprojects/Fabulous/samples/AllControls/AllControls/AllControls.fs(381,78): error FS0495: The member or object constructor 'Label' has no argument or settable return property 'styleClass'. The required signature is static member View.Label : ?text:string * ?horizontalTextAlignment:TextAlignment * ?verticalTextAlignment:TextAlignment * ?fontSize:obj * ?fontFamily:string * ?fontAttributes:FontAttributes * ?textColor:Color * ?formattedText:ViewElement * ?lineBreakMode:LineBreakMode * ?lineHeight:double * ?maxLines:int * ?textDecorations:TextDecorations * ?horizontalOptions:LayoutOptions * ?verticalOptions:LayoutOptions * ?margin:obj * ?gestureRecognizers:ViewElement list * ?anchorX:double * ?anchorY:double * ?backgroundColor:Color * ?heightRequest:double * ?inputTransparent:bool * ?isEnabled:bool * ?isVisible:bool * ?minimumHeightRequest:double * ?minimumWidthRequest:double * ?opacity:double * ?rotation:double * ?rotationX:double * ?rotationY:double * ?scale:double * ?translationX:double * ?translationY:double * ?widthRequest:double * ?resources:(string * obj) list * ?styles:Style list * ?styleSheets:StyleSheets.StyleSheet list * ?isTabStop:bool * ?scaleX:double * ?scaleY:double * ?tabIndex:int * ?childrenReordered:(EventArgs -> unit) * ?measureInvalidated:(EventArgs -> unit) * ?focused:(FocusEventArgs -> unit) * ?sizeChanged:(Fabulous.CustomControls.SizeChangedEventArgs -> unit) * ?unfocused:(FocusEventArgs -> unit) * ?visual:IVisual * ?classId:string * ?styleId:string * ?automationId:string * ?created:(Label -> unit) * ?ref:ViewRef -> ViewElement.

@@ -95,18 +95,6 @@
"name": "Scale",
"defaultValue": "1.0"
},
{
"name": "Style",
Copy link
Member

Choose a reason for hiding this comment

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

Is it supposed to be removed?

Copy link
Member

Choose a reason for hiding this comment

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

Along with StyleClass

Copy link
Collaborator Author

@SergejDK SergejDK May 22, 2019

Choose a reason for hiding this comment

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

I took it from fuget:

    public IList<string> class { get; set; }
    public Style Style { get; set; }
    public IList<string> StyleClass { get; set; }

In the Visualelement it looks like it does not exist anylonger

In the microsoftdoc: https://docs.microsoft.com/en-us/xamarin/xamarin-forms/release-notes/4.0/4.0.0-api#type-changed-xamarinformsvisualelement

Copy link
Member

Choose a reason for hiding this comment

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

It has been moved to NavigableElement now, which VisualElement inherits
https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.navigableelement?view=xamarin-forms

Copy link
Member

Choose a reason for hiding this comment

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

Comparing 3.4.0 vs 4.0.0, fuget shows that NavigableElement was created with these 3 properties
https://www.fuget.org/packages/Xamarin.Forms/4.0.0.425677/lib/netstandard2.0/diff/3.4.0.1008975/

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we will need to change the Generator to prevent exposing those abstract intermediate classes, though.
Otherwise we will polute the View.* with non-usable controls

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will change it. didn't see that it was moved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right that we should change the generator a bit. Was mentioned in a issue, too. But that will be another PR... First of all the upgrade to 4.0

@SergejDK
Copy link
Collaborator Author

The SearchBoxVisibility-Problem happens in the creation of the testnuget part.
I think that the problem here is that in the last version of XF they had a typo in it and now they corrected it. But the templates are using the "old" Fabulous version 0.34 which is using the SearchBoxVisiblity with the typo.
I will try to reference my local version.

@TimLariviere
Copy link
Member

./build.sh Test should generate a local nupkg for Fabulous and use it to run the samples.
So you shouldn't have a problem with SearchBoxVisibility when running this command.

@SergejDK
Copy link
Collaborator Author

The interesting part is that I get this error. Will push my latest changes in the next hour.

@SergejDK
Copy link
Collaborator Author

@TimLariviere
kinda works here. Maybe I changed sth. locally....

@TimLariviere
Copy link
Member

TimLariviere commented May 24, 2019

I also have the error with SearchBoxVisiblity.
I suspect it's because we're still using the same version number as present on nuget.org.
When the testapp is trying to restore a Fabulous v0.34.0, it is already in cache and NuGet restore won't search for a local package.

The easiest way to avoid that error is by temporarily changing the version number in the RELEASE_NOTES.md

@TimLariviere
Copy link
Member

@SergejDK I can't find BaseShellItem in the json file. Is it missing?
It has some properties like IsTabStop and TabIndex
https://docs.microsoft.com/en-us/xamarin/xamarin-forms/release-notes/4.0/4.0.0-api#type-changed-xamarinformsbaseshellitem

@SergejDK
Copy link
Collaborator Author

@TimLariviere
Honestly I don't know. Maybe i included it in 3.5 and deleted in 3.6 but don't know why. Currently on my phone and can't check easy where I throw it away.
Will include it.

For the searchboxvisibility problem we could write sth. In the docs for the time being. After a new release it should work just fine ?

Copy link
Member

@TimLariviere TimLariviere left a comment

Choose a reason for hiding this comment

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

Great job!
Except for these few remarks, everything is good. :)

{
"name": "Unfocused",
"defaultValue": "null",
"inputType": "Xamarin.Forms.FocusEventArgs -> unit",
Copy link
Member

Choose a reason for hiding this comment

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

{
"name": "Focused",
"defaultValue": "null",
"inputType": "Xamarin.Forms.FocusEventArgs -> unit",
Copy link
Member

Choose a reason for hiding this comment

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

Same for Focused

@TimLariviere
Copy link
Member

For the searchboxvisibility problem we could write sth. In the docs for the time being. After a new release it should work just fine ?

Yes. That would be a good idea.

@SergejDK
Copy link
Collaborator Author

@TimLariviere
added BaseShellItem and changed to EventArgs. I hope that will work fine. Didn't get the time to check it locally.

For the docs - could you write sth. there, please? - I'm out of town till sunday. Would be great so we can maybe merge it if everything works

@TimLariviere
Copy link
Member

All's good.
Merging now :)

@TimLariviere TimLariviere merged commit 87e2000 into fabulous-dev:master May 24, 2019
@TimLariviere TimLariviere mentioned this pull request May 25, 2019
@xperiandri
Copy link

@TimLariviere, maybe you would better bump version to 0.40 to indicate that it has some significant changes?

@xperiandri
Copy link

@SergejDK, works perfectly. Thank you very much for a great instant update.

@TimLariviere
Copy link
Member

@xperiandri In my opinion, even with this major update to support Xamarin.Forms 4.0, Fabulous remains mostly the same. There's only a bunch of new controls supported, which most updates of Fabulous add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants