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.4 #644

Merged
merged 11 commits into from
Feb 10, 2020
Merged

XF Upgrade 4.4 #644

merged 11 commits into from
Feb 10, 2020

Conversation

SergejDK
Copy link
Collaborator

@SergejDK SergejDK commented Dec 18, 2019

fix #641

TODO:

  • IndicatorView Implementation
  • SwipeView Implementation
  • SwipeItems Implementation
  • Use in AllControls
  • CarouselVertical

@SergejDK
Copy link
Collaborator Author

@TimLariviere
Should I add the new Views to AllControls ?

@TimLariviere
Copy link
Member

Should I add the new Views to AllControls ?

Yes, please.

@TimLariviere
Copy link
Member

Seems like there's a new LinearItemsLayout value like you added in #614: CarouselVertical
https://docs.microsoft.com/en-us/xamarin/xamarin-forms/release-notes/4.4/4.4.0-api#type-changed-xamarinformslinearitemslayout

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 so far!
I've added a few remarks on what's missing or what can prove challenging to correctly support.

@SergejDK
Copy link
Collaborator Author

SergejDK commented Dec 18, 2019

@TimLariviere
Good points :)

I see that we have an CustomEntryCell where the Text property and the textchanged event is used. Should we remove those parts? Is there even a need for a customEntryCell anylonger?

EDIT:
It is needed because there the textchangedevent is handled. But I think we can remove the two not used parts.

Just saw that it is Cell and not normal Entry....

@TimLariviere
Copy link
Member

I see that we have an CustomEntryCell where the Text property and the textchanged event is used. Should we remove those parts? Is there even a need for a customEntryCell anylonger?

Yes, we need to keep it.
EntryCell doesn't inherit from InputView so it declares its own properties and events.
We had to write a CustomEntryCell because EntryCell only provide an overridable OnTextChanged instead of an event that we need.

@SergejDK
Copy link
Collaborator Author

@TimLariviere
I was taking a look on how to use IndicatorView and somehow I think we do not need to use ItemsSource and IndicatorTemplate.

https://docs.microsoft.com/en-us/xamarin/xamarin-forms/user-interface/indicatorview

Normally the method ItemsSourceBy is used with a reference to a CarouselView.

What do you think about exposing this function as a event and accept a ViewRef as in Input?

@TimLariviere
Copy link
Member

TimLariviere commented Dec 19, 2019

What do you think about exposing this function as a event and accept a ViewRef as in Input?

Could you explain a bit more why you see it as an event?
But I agree that ViewRef<CarouselView> seems our best option, even though I'm not a fan...

@SergejDK
Copy link
Collaborator Author

SergejDK commented Dec 19, 2019

@TimLariviere
did it as an property - don't know why I was thinking about events ....

One thing that I caught was that using a ViewRef for CarouselView needs a let carouselViewRef = ViewRef<CustomCarouselView> instead of ViewRef<CarouselView>. Maybe we should look at this ?

I think I need to rewrite the updateIndicatorViewRef function so it is possible to have params like prevValueOpt: ViewRef<'T> voption instead of a specific type. - What do you think?

After all this is working with the ViewRef idea.

@SergejDK
Copy link
Collaborator Author

@TimLariviere
SwipeView is working fine and IndicatorView, too :) Samples are included in AllControls.

@SergejDK SergejDK marked this pull request as ready for review December 29, 2019 21:04
@SergejDK
Copy link
Collaborator Author

SergejDK commented Jan 2, 2020

@TimLariviere
do you have a better solution for CarouselVerticalItemsLayout than I currently have?

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.

Sorry for the late reply. I needed some time off.
I'm slowly getting back on this PR. Here's a few things I found.

| ValueSome prevValue, ValueSome currValue when prevValue = currValue -> ()
| ValueNone, ValueNone -> ()
| _, ValueSome currValue -> Shell.SetNavBarHasShadow(target, currValue)
| ValueSome _, ValueNone -> Shell.SetNavBarHasShadow(target, true)
Copy link
Member

@TimLariviere TimLariviere Feb 2, 2020

Choose a reason for hiding this comment

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

The default value of NavBarHasShadow is dependent of the platform. If it's Android, it is true, false otherwise.
See https://github.com/xamarin/Xamarin.Forms/blob/1ae8fb1cd53f8cc0584249f824e457ce2a091354/Xamarin.Forms.Core/Shell/Shell.cs#L37

match currValue.TryValue with
| Some v -> IndicatorView.SetItemsSourceBy(target, v)
| None -> ()
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 should set the ItemsSource to null in the case the ref is invalid, instead of doing nothing and keeping a previous state no longer wanted?

{
"source": null,
"name": "ShellNavBarHasShadow",
"defaultValue": "true",
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Default value is dependent of the platform.

@TimLariviere
Copy link
Member

TimLariviere commented Feb 2, 2020

do you have a better solution for CarouselVerticalItemsLayout than I currently have?

@SergejDK There doesn't seem to be a better way. Relying directly on LinearItemsLayout.CarouselLayout would lead to accidental overriding of the value since it's a class.

@SergejDK SergejDK changed the title WIP: XF Upgrade 4.4 XF Upgrade 4.4 Feb 3, 2020
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.

Thanks!

@TimLariviere TimLariviere merged commit a5c2051 into fabulous-dev:master Feb 10, 2020
@magnushammar
Copy link

Awesome work thanks!

I just compiled an app with it now and my SwipeItems are showing as expected.

@SergejDK SergejDK deleted the 44 branch February 13, 2020 14:59
@Happypig375 Happypig375 mentioned this pull request Mar 1, 2020
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.

SwipeView/IndicatorView
3 participants