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

App pretty weather sample #605

Merged

Conversation

chrisevans9629
Copy link

Pretty Weather App

I created a new Xamarin.Forms sample based on James Montemagno's pretty weather app

This is related to adding samples #242

image

image

@TimLariviere
Copy link
Member

TimLariviere commented Nov 14, 2019

@chrisevans9629 For now, I've only resolved the conflict to make sure it builds properly and is mergeable.
Will review the code later

@TimLariviere
Copy link
Member

Oh, and I renamed it FabulousWeather... :)

@chrisevans9629
Copy link
Author

An adequate name 😄

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 for your work! It really is a nice sample :)

I added some comments to make sure it is on-par with the other samples before we merge it.

@chrisevans9629
Copy link
Author

I have added the other projects and got the actual api working, just working on creating a simple renderer for the other platforms so that the pancakeview will work with them.

@TimLariviere
Copy link
Member

TimLariviere commented Nov 18, 2019

@chrisevans9629 Ah, if some platforms don't support PancakeView officially, you don't need to implement it.
We will just put those platforms aside for this sample!

For example, the Fabimals sample only target Android and iOS because the Shell control is only available on those.

@Happypig375
Copy link

@TimLariviere Shell and CollectionView are now available on UWP. Please update.

@Happypig375
Copy link

@TimLariviere
Copy link
Member

@Happypig375 Could you open a separate issue so we can track the progress of updating the related samples please?

@chrisevans9629
Copy link
Author

chrisevans9629 commented Nov 19, 2019

@TimLariviere I think have completed all the requested changes. Looks like the build is failing due to FSharp.Core. Do you know how to fix that?

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! I love that you used the JSON type provider.
Added some other comments, it should fix the build issue too.

I'm still waiting for my API key to become active before I can really try the app.

@chrisevans9629
Copy link
Author

Done! Looks like it's building now

@dsyme
Copy link
Collaborator

dsyme commented Nov 21, 2019

Christmas wish: It would be awesome if all the samples were also available as templates in the templates pack :)

Copy link
Collaborator

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Just a few comments about code formatting. It's good if the code in this sample is nice and pretty since it can be used as a like-for-like comparison with MVVM C#

@TimLariviere
Copy link
Member

The iOS project was behaving weirdly, so I made sure everything was in place and now it works perfectly.

@TimLariviere
Copy link
Member

Christmas wish: It would be awesome if all the samples were also available as templates in the templates pack :)

If Santa Claus could bring Xamarin SDK-style projects or support for PackageReference in old-style F# projects, that would make it a whole lot easier to do :)

Currently, what's missing is a good way to handle centralized dependency versioning across the repo between old and SDK-style projects.

Paket could fill that hole, but not if we want to ship the samples independently (or maybe I'm missing something?).

@dsyme
Copy link
Collaborator

dsyme commented Nov 21, 2019

If Santa Claus could bring Xamarin SDK-style projects or support for PackageReference in old-style F# projects, that would make it a whole lot easier to do :)

Yup, agreed

@chrisevans9629
Copy link
Author

Paket could fill that hole, but not if we want to ship the samples independently (or maybe I'm missing something?).

I notice in the projects that there is a lot of the use of "..\..\..\..\packages" to get to the packages folder. What if we made that path a build variable so it could be easily changed for the samples? Maybe as part of the build there could be an option to copy the samples into a new folder with it's own separate version of packet? To detach the samples from the project references, maybe we could have a local nuget folder with the Fabulous and liveupdate nugets that packet references? That way they could be copied and still be referencing the most current version.

Doesn't seem to be an easy way to do it, but I'll tinker around with this in a different branch

@chrisevans9629
Copy link
Author

Maybe if we did something like this in the packet dependencies:

source https://api.nuget.org/v3/index.json
source .\Nugets
nuget Fabulous.XamarinForms
nuget Fabulous.XamarinForms.LiveUpdate

The nugets could be generated on build
Then we could do this in the project file:

    <PackagesPath>..\..\..\..\</PackagesPath>
    <EnableDefaultEmbeddedResourceItems>false</EnableDefaultEmbeddedResourceItems>
  </PropertyGroup>
  <Import Project="$(PackagesPath)Packages.targets" />

  <ItemGroup>
    <Compile Include="PancakeViewExtensions.fs" />
    <Compile Include="App.fs" />
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="FSharp.Data" />
    <PackageReference Include="Xamarin.Forms.PancakeView" />
    <PackageReference Include="Fabulous.XamarinForms"/>
    <PackageReference Include="Fabulous.XamarinForms.LiveUpdate"/>
  </ItemGroup>

Then the samples could be copied and we could maybe replace PackagesPath with whatever we need. Since there is no reference to the projects, users could just do paket install and it would get the packages from the local nuget folder or from nuget.org

Could that work?

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.

Looking good!
I added the missing image for UWP and reformatted the code a bit to uniformize the styles

@TimLariviere TimLariviere merged commit 8067bbe into fabulous-dev:master Nov 25, 2019
@chrisevans9629 chrisevans9629 deleted the appPrettyWeatherSample branch November 25, 2019 23:32
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.

None yet

4 participants