Improve the usage experience with IsInRangeConverter in XAML

[GeorgeLeithead]

Description of Change

When initially written, the IsInRangeConverter used CompareConverter as a guide/basis. Since then, the CompareConverter has been changed to improve the usage experience in XAML. See #1841 for details.

This PR brings the IsInRangeConverter back into alignment with the CompareConverter, in terms of implementation.

The current use of the converter is as follows:

            <Color x:Key="LightGreen">LightGreen</Color>
            <Color x:Key="PaleVioletRed">PaleVioletRed</Color>
            <x:Double x:Key="MinValue">3.0</x:Double>
            <x:Double x:Key="MaxValue">7.0</x:Double>
            <converters:DoubleIsInRangeConverter
                x:Key="isBetweenThreeAndSevenConverter"
                FalseObject="{StaticResource PaleVioletRed}"
                MaxValue="{StaticResource MaxValue}"
                MinValue="{StaticResource MinValue}"
                TrueObject="{StaticResource LightGreen}" />

If a developer tries to write MaxValue="7", and the value for the converter is a Double, the user will not get any valid results from the converter. In XAML, the MaxValue="7" has a type of String, by default.

This PR makes it possible to define what type MaxValue and MinValue should be when inheriting from the converter, based on the value.

Converter definition

public sealed class DoubleIsInRangeConverter : IsInRangeConverter<double, Color>;

XAML Usage

            <converters:DoubleIsInRangeConverter
                x:Key="isBetweenThreeAndSevenConverter"
                FalseObject="{StaticResource PaleVioletRed}"
                MaxValue="7"
                MinValue="3"
                TrueObject="{StaticResource LightGreen}" />

I have also removed the un-used CompareValue property.

I think that this will make is easier for users to use, and eliminate any confusion about XAML defaulting to String for this control.

Linked Issues

• Fixes #

PR Checklist

• Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Changes adhere to coding standard
• Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

[GeorgeLeithead]

Note that I would need to update the DOCs, so this should be blocked until the docs are ready.

[GeorgeLeithead]

This also "Needs Discussion" label added.

[bijington]

@GeorgeLeithead thank you for this! Apologies for the delay in reviewing this. I have added a number of comments, some improving existing bits (xml docs) and others just to confirm my understanding. I like the concept.

[bijington]

Thank you for this @GeorgeLeithead

[brminnick]

Are we sure that removing the all of the BindablePropertys is a good choice?

This means that users can no longer bind to any of the following properties from the VIewModel:

• FalseObject
• MaxValue
• MinValue
• TrueObject

Personally, I'd prefer that we keep the bindable properties unless someone with more XAML knowledge can help me understand.

[bijington]

Are we sure that removing the all of the BindablePropertys is a good choice?

This means that users can no longer bind to any of the following properties from the VIewModel:

• FalseObject

• MaxValue

• MinValue

• TrueObject

Personally, I'd prefer that we keep the bindable properties unless someone with more XAML knowledge can help me understand.

That was my initial thinking but I struggled to get input on this.

[bijington]

Let me put them back so we can merge this in

[GeorgeLeithead]

I have some comments to make on this. Well do so tomorrow.

[GeorgeLeithead]

Um.. Ok, too late to add my 2-pennies... but...

• The original implementation I made for IsInRangeConverter was based on the ONLY other generic converter in the toolkit CompareConverter. When it was upgraded in PR Improve the usage experience with CompareConverter in XAML #1841 , this therefore prompted me to 'do the same' to this converter. As such, the bindable properties were not present in the CompareConverter.
• All of the samples, tests, examples in the docs AND the examples I provided in the PR details, all work and in fact it's now much easier now as it's easier to provide the TYPE.
• Currently, I believe that no other converter in the tool kit provides any BindableProperty(s).

So, I think the big question in reality is: Should we either adhere to the same standard and NOT have bindable properties, or should we add bindable properties to all [appropriate] converters? If the former, then I don't think we should have bindable properties on this converter. If the latter, then IMO a bigger PR should be undertaken, and at the (very IMO) VERY LEAST additional Unit Tests should have been added BEFORE we approved the PR.

Thoughts? @bijington @brminnick, et all? (Should this be an open discussion on Discord, instead of against the PR?)

[bijington]

I agree that we should consider the bigger picture on whether all or no converters should have BindableProperty's or none.

I don't agree that additional tests should have been added before this PR merged, the point that I believe is important is that the PR was removing functionality by removing the BindableProperty definitions, the last change was just to prevent this loss of functionality - this wasn't adding new functionality that needed to also be tested. Although in truth those tests should have existed which would have caught this loss of functionality but hindsight is a wonderful thing.

I completely agree with the below statement and thank you for improving it

All of the samples, tests, examples in the docs AND the examples I provided in the PR details, all work and in fact it's now much easier now as it's easier to provide the TYPE.

The above is still true with the retention of the BindableProperty's.

But to come back to the original point, I agree that we should discuss whether all or no converters should have bindable properties and we consider the risk of breaking people based on the outcome.