Skip to content

Conversation

0pd
Copy link
Contributor

@0pd 0pd commented Oct 31, 2016

I extended converters the way we use them at work.

Also BindableValueHolder is very helpful thing when you use VisualState.Setters.

@dnfclas
Copy link

dnfclas commented Oct 31, 2016

Hi @llvk, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

Copy link
Contributor

@ScottIsAFool ScottIsAFool left a comment

Choose a reason for hiding this comment

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

Can you include some docs/explanation about what the BindableValueHolder is for and how to use it?

@dnfclas
Copy link

dnfclas commented Oct 31, 2016

@llvk, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@0pd
Copy link
Contributor Author

0pd commented Oct 31, 2016

@ScottIsAFool of course

@0pd
Copy link
Contributor Author

0pd commented Oct 31, 2016

@ScottIsAFool done.

/// <returns>The value to be passed to the target dependency property.</returns>
public virtual object Convert(object value, Type targetType, object parameter, string language)
{
var isEmpty = value == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not make more sense to have something like
var isEmpty = CheckValueIsEmpty(value);
where CheckValueIsEmpty is a virtual method. Given everything else in the subsequent converters is exactly the same other than this one line, would make more sense, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems clearer.

@0pd
Copy link
Contributor Author

0pd commented Oct 31, 2016

@ScottIsAFool Can I squash the branch?

@ScottIsAFool
Copy link
Contributor

@llvk Sure.

Make BoolToObjectConverter, EmptyCollectionToObjectConverter and EmptyStringToObjectConverter to inherit DependencyObject.
Add backing DependencyProperties for all the values of converters mentioned above.
Add BindableValueHolder.
@lukasf
Copy link
Contributor

lukasf commented Oct 31, 2016

I like the BindableValueHolder. I used to create custom classes for holding stuff like this.

@deltakosh
Copy link
Contributor

Really great idea!
LGTM

@deltakosh deltakosh merged commit 4794295 into CommunityToolkit:dev Oct 31, 2016
@0pd 0pd deleted the improvement/converters branch October 31, 2016 21:31
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.

5 participants