Skip to content

Conversation

@glennawatson
Copy link
Contributor

@glennawatson glennawatson commented Sep 22, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
It's a bug fix for: #1164

What is the current behavior? (You can also link to an open issue here)
#1164

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
Don't believe so, just uses the existing view model When Any Value overloads used by the vmProperty field. This allows the ViewModel to be null initially.

Please check if the PR fulfills these requirements

Other information:

…TParam>> withParameter' overload when having initially a null ViewModel property

@ghuntley ghuntley closed this Nov 7, 2016
@ghuntley ghuntley changed the base branch from rxui7-master to develop November 7, 2016 00:20
@ghuntley ghuntley reopened this Nov 7, 2016
@ghuntley ghuntley requested a review from kentcb January 9, 2017 08:17
@ghuntley
Copy link
Member

ghuntley commented Jan 9, 2017

Thanks for the PR Glenn. @reactiveui/reviewers-core can you please review.

@kentcb
Copy link
Contributor

kentcb commented Jan 10, 2017

@glennawatson can you provide an example of code that previously failed that will now work with this PR applied?

I'm a little unclear from reading the PR description. It makes it sound like it's merely fixing existing behavior, but in fact the PR adds a whole new BindCommand overload. My understanding is that you've added the overload to facilitate delayed evaluation of the VM. Is that correct? Regardless, can you please provide some sample before/after code.

@glennawatson
Copy link
Contributor Author

Sorry been away of late. Will get you a example in the next day or so.

@kentcb
Copy link
Contributor

kentcb commented Feb 21, 2017

@glennawatson ping

@glennawatson
Copy link
Contributor Author

Hi,

Not going to be able to get back to you guys for at least a week. Been in the hospital for a bit.

The main issue was using the Expression<Func<>> form, it didn't take into account changing values

ex thisValue => thisValue.VM.Blah

If VM wasn't set initially it wouldn't trigger properly.

So I changed it to use the same mechanism you use in the other code to handle that case.

@glennawatson
Copy link
Contributor Author

glennawatson commented Feb 22, 2017

If you look internally also, I added a new private method but changed the public facing method to use the new private method to use the better approach of using the new method that uses the following:

var withParamExpression = Reflection.Rewrite(withParameter.Body);
var withParamFunc = Reflection.ViewModelWhenAnyValue(viewModel, view, withParamExpression);

@glennawatson
Copy link
Contributor Author

Anyway I'm not going to be able to help change the code sample for the next week or so due to technical restrictions, but hope that explanation helps.

@kentcb
Copy link
Contributor

kentcb commented Feb 22, 2017

Thanks @glennawatson! I wish you a swift recovery.

If I get a chance I'll take a look at getting this merged for the next release.

@dnfclas
Copy link

dnfclas commented Aug 25, 2017

@glennawatson,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@glennawatson
Copy link
Contributor Author

I signed the CLA btw, hasn't been updated here though since I did it over a week ago.

…ow for the case of having a null view model at the time of binding
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bb03ef8 on glennawatson:rxui7-master into ** on reactiveui:develop**.

@glennawatson
Copy link
Contributor Author

I updated with the latest develop to pass the tests.

@glennawatson
Copy link
Contributor Author

Closing due to CLA bot issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants