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

housekeeping: Update cinephile #1973

Merged
merged 30 commits into from Mar 17, 2019

Conversation

Projects
None yet
5 participants
@giusepe
Copy link
Member

giusepe commented Mar 15, 2019

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Sample Update

What is the current behavior? (You can also link to an open issue here)
The sample is using ReactiveList, a single project for Views and ViewModels and lacks in the unit tests for ViewModels

What is the new behavior (if this is a feature change)?
Cinephile now uses Dynamic data
The main project was split into: Views and ViewModels, it helps to keep it clean of errors (referencing Views on VMs and such. Also makes it easier to test)
Added some unit tests for the ViewModels

What might this PR break?
Cinephile sample

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
I had to reopen the Pr because #1972 closed due to a force push :)

giusepe and others added some commits Feb 15, 2019

Giusepe Casagrande
Lots of small refactorings
Added a display alert
Imrpoved the view and viewmodel base
 
Merge branch 'master' into update-cinephile
 
Update samples/xamarin-forms/Cinephile/iOS/Cinephile.iOS.csproj

Co-Authored-By: glennawatson <glenn@glennwatson.net>

@giusepe giusepe requested review from glennawatson and alexmartinezm Mar 15, 2019

@giusepe giusepe requested a review from reactiveui/core-team as a code owner Mar 15, 2019

@giusepe giusepe changed the title Update cinephile housekeeping: Update cinephile Mar 15, 2019

@alexmartinezm
Copy link
Contributor

alexmartinezm left a comment

Well done @giusepe, but remember to fix the code following analyzers tips (eg. prefix local calls with "this"). And thanks for keeping this updated!

@glennawatson

This comment has been minimized.

Copy link
Contributor

glennawatson commented Mar 15, 2019

Well done @giusepe, but remember to fix the code following analyzers tips (eg. prefix local calls with "this"). And thanks for keeping this updated!

If you're getting asked for prefix 'this' then the code analyzer ruleset file isn't getting picked up properly. We use the _notation

@alexmartinezm

This comment has been minimized.

Copy link
Contributor

alexmartinezm commented Mar 15, 2019

Well done @giusepe, but remember to fix the code following analyzers tips (eg. prefix local calls with "this"). And thanks for keeping this updated!

If you're getting asked for prefix 'this' then the code analyzer ruleset file isn't getting picked up properly. We use the _notation

Not in the case of "ViewModel" property from a ViewFor implementation.

@glennawatson

This comment has been minimized.

Copy link
Contributor

glennawatson commented Mar 17, 2019

Retrigger CI by close/open

@glennawatson glennawatson merged commit 9898de9 into master Mar 17, 2019

1 check passed

license/cla All CLA requirements met.
Details

@delete-merged-branch delete-merged-branch bot deleted the update-cinephile branch Mar 17, 2019

glennawatson added a commit that referenced this pull request Mar 19, 2019

glennawatson added a commit that referenced this pull request Mar 19, 2019

glennawatson added a commit that referenced this pull request Mar 23, 2019

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.