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

Add the Fabimals sample #450

Merged
merged 9 commits into from Jun 18, 2019

Conversation

Projects
None yet
2 participants
@TimLariviere
Copy link
Member

commented Jun 12, 2019

Now that #446 and #447 are merged, the Fabimals sample can finally be added to Fabulous.

It is a direct port of the official Xaminals sample, to demonstrate how to use Shell in Fabulous.
https://github.com/xamarin/xamarin-forms-samples/blob/master/UserInterface/Xaminals

I feel like the implementation is ok, except for Routing which is currently being reworked in Xamarin.Forms (xamarin/Xamarin.Forms#5166)
But don't hesitate to suggest ways I can improve this sample.

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

/azp run full build

@azure-pipelines

This comment has been minimized.

Copy link

commented Jun 12, 2019

Azure Pipelines successfully started running 1 pipeline(s).
@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@TimLariviere
is this the last PR of the big one?

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Yes it is.

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

The code size is roughly 750 lines of F# (without accounting for the Data folder).

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@TimLariviere
could you maybe delete the Shell-Part in the AllControls because it is not needed anylonger cause auf your great example!
(Currently checking)

The code size is roughly 750 lines of F# (without accounting for the Data folder).

Not too bad.

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@TimLariviere
tested it so far - didn't look at the code yet! It works, thats great!

It is kind of laggy on my device in debug mode but I think this was discussed a lot in the issues - "Performance while navigating" etc. so we have this covered there.

I needed to reference the fabimalsproject in Fabimals.Droidproject again - so it looks like this for me now:
grafik

@SergejDK
Copy link
Collaborator

left a comment

All in all it looks good! Thanks for the example.
More or less I only had some questions in some parts :)

Interesting part is that the collectionview works in this example.... Would be interesting to find out why.

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

I needed to reference the fabimalsproject in Fabimals.Droidproject again - so it looks like this for me now

Should be fixed now

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Should be noted that using the search bar on a real iPhone currently crashes.
Opened an issue for that: xamarin/Xamarin.Forms#6505

View.SearchHandler(
placeholder="Enter search term",
showsResults=true,
queryChanged=(fun (_, newValue) -> dispatch (QueryChanged newValue)),

This comment has been minimized.

Copy link
@SergejDK

SergejDK Jun 13, 2019

Collaborator

Do you think we can debounce this?
I mean that not on every change the message is run - just curious if it is possible.

This comment has been minimized.

Copy link
@TimLariviere

TimLariviere Jun 13, 2019

Author Member

It is possible, yes. It's quite easy:

queryChanged=debounce 250 (fun (_, newValue) -> dispatch (QueryChanged newValue)),

I tried it with different times, but the UX doesn't feel satisfying.
The delay in the UI is not great.

This comment has been minimized.

Copy link
@SergejDK

SergejDK Jun 13, 2019

Collaborator

hmmm... does every message run or only the latest with the debounce ?
maybe if only the latest massage will run it could be a bit better.

This comment has been minimized.

Copy link
@TimLariviere

TimLariviere Jun 14, 2019

Author Member

debounce only sends the last message in a given time frame.
Everytime the event is triggered, it will wait for 250ms to check if there's no other event triggers. Only then it really calls the function and dispatches.

So this delay is kinda bad when typing. It can give a few quirks like if you're typing too slow, it will show delayed results not matching what you're currently typing.
E.g. You're typing "ABCD" slowly. While you wrote "ABC", it will display results for "AB".

So it's confusing.

This comment has been minimized.

Copy link
@SergejDK

SergejDK Jun 14, 2019

Collaborator

well i think maybe playing a bit with the timeframe could be a good way. I mean RX or ReactiveUI uses nearly the same logic for throttle or sth. else. There it works great, so I thought this could be a great thing to have.

Maybe we need to investigate on this and make mit more usable -> new issue?

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

@TimLariviere
looked through the code and it looks good to me.

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

Saw this on stream.
Bildschirmfoto 2019-06-14 um 19 35 43

The first item looks kinda broken. Didn't saw when it broke - I think after the search?

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

@SergejDK Yes, I did open an issue on the XF repo for that.
xamarin/Xamarin.Forms#6507

When you leave the page with a CollectionView and come back to it, for some reasons, the previous items are still there stuck at the top.

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

@TimLariviere
good thing!

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

I want to try to make it work with ShellContent.ContentTemplate instead of Content before merging it.

@SergejDK

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

Do you mean it as we do for listview?

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Yes. Just like we discussed it on the stream. Through the use of IShellContentController.
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/Shell/IShellContentController.cs

@TimLariviere

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

/// DataTemplate that can inflate a View from a ViewElement instead of a Type
type ViewElementDataTemplate(viewElement: ViewElement) as self =
inherit DataTemplate()
do (self :> Xamarin.Forms.Internals.IDataTemplate).LoadTemplate <- Func<obj>(viewElement.Create)

@PureWeen Would you know a better way to achieve this?
Xamarin.Forms.Internals.IDataTemplate is marked obsolete, but it seems to be the only way to change how the view should be constructed by a DataTemplate.

Whoops. Nevermind.
Missed the constructor of DataTemplate that accepts a function. 😄

/// DataTemplate that can inflate a View from a ViewElement instead of a Type
type ViewElementDataTemplate(viewElement: ViewElement) =
    inherit DataTemplate(Func<obj>(viewElement.Create))

@TimLariviere TimLariviere merged commit 7e14c74 into fsprojects:master Jun 18, 2019

3 checks passed

PR Build Build #20190618.2 succeeded
Details
PR Build (Windows) Windows succeeded
Details
PR Build (macOS) macOS succeeded
Details

@TimLariviere TimLariviere deleted the TimLariviere:fabimals-sample branch Jun 18, 2019

@TimLariviere TimLariviere referenced this pull request Jun 19, 2019

Merged

0.36.0 #463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.