-
Notifications
You must be signed in to change notification settings - Fork 381
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
Improve the usage experience with CompareConverter in XAML #1841
Conversation
I like it! Should we update the docs to demonstrate this as the recommended way going forward to use |
Yes I'll update the docs to reflect this 👍 |
Right on! I've added the
approved
It looks like there's a missing XML comment somewhere causing the build to fail, but once that's fixed (and the docs are generated) I'm happy to approve + merge 🙌 |
Awesome thanks @brminnick. Technically this change is breaking because It went from public abstract class CompareConverter<TReturnObject> : BaseConverterOneWay<IComparable, object> to public abstract class CompareConverter<TValue, TReturnObject> : BaseConverterOneWay<TValue, object> where TValue : IComparable If we want to avoid a major version now and breaking developers straightaway we could add in the following and mark it as [Obsolete]
public abstract class CompareConverter<TReturnObject> : CompareConverter <IComparable, TReturnObject> This would provide a more gentle break for those that are already using CompareConverter and not having an issue. What do you think? |
Good catch! I've added the
breaking change
Nah, don't worry about it. We recently merged another breaking change in #1839, so our next release will be a major version bump anyways. We'll both just want to double check the Release Notes to ensure these PRs with the
breaking change
|
Docs are at: MicrosoftDocs/CommunityToolkit#415 |
Description of Change
The aim of this PR is to simplify the usage of the
CompareConverter
through better control of implementation when inheriting formCompareConverter
.The current usage of the converter is as follows:
If a developer tries to write
ComparingValue="0.5"
they will receive a compilation error stating:This is down to the fact that
ComparingValue
is of typeIComparable
and there is no implicit conversion that can be determined between the value 0.5 (which is astring
) toIComparable
.This PR makes it possible to define what type
ComparingValue
should be when inheriting from the converter. Therefore usage becomes:Converter definition
XAML usage
It might feel like a small improvement but I believe it will make quite a difference, especially when using the markup extension to define it inside the binding. The definitions become a lot more concise.
Question
Should we go a step further and make
ComparingValue
aBindableProperty
and allow developers to make the value dynamic through the use of bindings? I think yes but I would like to see what others think.Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information