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

TimLariviere
Copy link
Member

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
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@SergejDK
Copy link
Collaborator

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

@TimLariviere
Copy link
Member Author

Yes it is.

@TimLariviere
Copy link
Member Author

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

@SergejDK
Copy link
Collaborator

@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
Copy link
Collaborator

@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

Copy link
Collaborator

@SergejDK SergejDK left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

Should be fixed now

@TimLariviere
Copy link
Member Author

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)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@TimLariviere TimLariviere Jun 13, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

SergejDK commented Jun 13, 2019

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

@SergejDK
Copy link
Collaborator

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
Copy link
Member Author

@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
Copy link
Collaborator

@TimLariviere
good thing!

@TimLariviere
Copy link
Member Author

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

@SergejDK
Copy link
Collaborator

Do you mean it as we do for listview?

@TimLariviere
Copy link
Member Author

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
Copy link
Member Author

TimLariviere commented Jun 18, 2019

https://github.com/fsprojects/Fabulous/blob/b13e7c66f27a0ef754db7afb19afca3dacc9fdcb/src/Fabulous.Core/ViewConverters.fs#L203-L206

@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 fabulous-dev:master Jun 18, 2019
@TimLariviere TimLariviere deleted the fabimals-sample branch June 18, 2019 20:48
@TimLariviere TimLariviere mentioned this pull request Jun 19, 2019
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.

2 participants