IsInRangeConverter

[GeorgeLeithead]

Description of Change

Implement the proposed IsInRangeConverter.

Linked Issues

#65

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: Added IsInRangeConverter documentation. MicrosoftDocs/CommunityToolkit#176

Additional information

Tested on:

• Android
• Windows
• iOS
• MacCatalyst

[VladislavAntonyuk]

The issue is not championed

[GeorgeLeithead]

The issue is not championed

My appologies, I jumped the gun. Was ensuring that before my time becomes more restricted starting next week, any work I have done was made available.
Thankfully the other 5 items I haven't created a PR yet, as they too have not yet been championed!

Have been discussing and sharing this with @bijington and didn't check to see if the championed status had been updated.

[brminnick]

Closing PR until Proposal has been Approved

[brminnick]

Reopening after Proposal Approval

[GeorgeLeithead]

@JoonghyunCho @bijington This is ready for review. Any suggestions, improvements, additions would be very much appreciated. Documentation is also needed, which I will work on!

[GeorgeLeithead]

@JoonghyunCho @bijington This is ready for review. Any suggestions, improvements, additions would be very much appreciated. Documentation is also needed, which I will work on!

Please note that I do not have access to any iOS or Mac Catalyst devices (or Tizen), as such, please make sure that any reviewer who does have access, to please update as appropriate the checkbox indicating that it works on that platform.

[JoonghyunCho]

I will check if it works ok on iOS and Mac devices later tonight :)

[GeorgeLeithead]

Also, please be aware that I also need to create the appropriate test cases.

[JoonghyunCho]

LGTM

[brminnick]

Thanks George!

Just FYI - I updated the sample app to use a single Grid for its layout. The initial implementation had Grids nested inside of HorizontalStackLayouts nested inside of VerticalStackLayouts.

don't attempt to reproduce the appearance of a specific layout by using combinations of other layouts, as this results in unnecessary layout calculations being performed. For example, don't attempt to reproduce a Grid layout by using a combination of StackLayout instances.

https://learn.microsoft.com/xamarin/xamarin-forms/deploy-test/performance#choose-the-correct-layout

Because of the large number of Rows in the Grid, I ported the IsInRangeConverterPage to C# Markup which allows the naming of rows + columns. This makes it easier to assign UI controls to their respective column + row and helps avoid confusion amongst which row is which in the Grid.

[brminnick]

Approving again after adding the Bindable Properties defined in #65

[GeorgeLeithead]

Approving again after adding the Bindable Properties defined in #65

@brminnick I used the existing converters as a guide for implementation, especially CompareConverter.shared.cs as it was close to what IsInRangeConverter was to provide. If I KNEW that Bindabale Properties was required, or given time to implement myself, I would have very VERY much have preferred to do so myself!
However, I do also find it very strange that Bindable Properties have been added to this Converter but not to the other existing converters, if they are now 'required'. Especially for from a consistent end user experience.

[GeorgeLeithead]

Approving again after adding the Bindable Properties defined in #65

@brminnick I used the existing converters as a guide for implementation, especially CompareConverter.shared.cs as it was close to what IsInRangeConverter was to provide. If I KNEW that Bindabale Properties was required, or given time to implement myself, I would have very VERY much have preferred to do so myself!
However, I do also find it very strange that Bindable Properties

Thanks George!

Just FYI - I updated the sample app to use a single Grid for its layout. The initial implementation had Grids nested inside of HorizontalStackLayouts nested inside of VerticalStackLayouts.

don't attempt to reproduce the appearance of a specific layout by using combinations of other layouts, as this results in unnecessary layout calculations being performed. For example, don't attempt to reproduce a Grid layout by using a combination of StackLayout instances.

https://learn.microsoft.com/xamarin/xamarin-forms/deploy-test/performance#choose-the-correct-layout

Because of the large number of Rows in the Grid, I ported the IsInRangeConverterPage to C# Markup which allows the naming of rows + columns. This makes it easier to assign UI controls to their respective column + row and helps avoid confusion amongst which row is which in the Grid.

@brminnick Originally it started off as 1 sample that I hoped to make multi-use. However it didn't pan out that way. I should have converted to be a grid, and would have been happy to do the work, if I was given a chance to do so from a review.

[brminnick]

No worries at all, George! You always do fantastic work and we're honored to have you as a strong contributor ❤️

The BindableProperties were in the API Design for the approved Proposal, and I wanted to ensure we honored that spec. I agree it is a bit inconsistent with other Converters today, but I think it opens up an awesome opportunity to add BindableProperties to existing Converters!

And your sample was great too! Since I know many devs will copy/paste our samples, I just wanted to ensure we start them off on the right path using best practices. You'd be surprised how many times I've been blamed in the past because I wrote some quick + dirty code as an example, only to find another dev copy/pasted it and made it the foundation of their app 😳

[GeorgeLeithead]

No worries at all, George! You always do fantastic work and we're honored to have you as a strong contributor ❤️

The BindableProperties were in the API Design for the approved Proposal, and I wanted to ensure we honored that spec. I agree it is a bit inconsistent with other Converters today, but I think it opens up an awesome opportunity to add BindableProperties to existing Converters!

And your sample was great too! Since I know many devs will copy/paste our samples, I just wanted to ensure we start them off on the right path using best practices. You'd be surprised how many times I've been blamed in the past because I wrote some quick + dirty code as an example, only to find another dev copy/pasted it and made it the foundation of their app 😳

@brminnick Have you seen all the Unit Tests I put in? :-D
For the Bindable Properties, could be raised as a PROPOSAL, and be an opportunity to bump up the samples, and an opportunity to update the documentation to provide more extensive doc examples.