Adding control size trigger

[dpaulino]

Fixes

Adding a new visual state trigger which can be triggered based on a the width of a UI element rather than the width of the app's entire window.

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link: Addings docs for new ControlSizeTrigger MicrosoftDocs/WindowsCommunityToolkitDocs#551
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

[ghost]

Thanks dpaulino for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[net-foundation-cla]

All CLA requirements met.

[net-foundation-cla]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ dpaulino sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[michael-hawker]

Thanks @dpaulino, I feel like we may have an issue somewhere open for this? I know it's been a long-standing 'to do' thing. 🙂

You can check-out the Wiki Page on Adding a Sample here too, we may also want to add tests for the triggers in our test suite.

[dpaulino]

Thanks dpaulino, I feel like we may have an issue somewhere open for this? I know it's been a long-standing 'to do' thing. 🙂

You can check-out the Wiki Page on Adding a Sample here too, we may also want to add tests for the triggers in our test suite.

I took a glance at issues relating triggers but couldn't spot one. But I can try to look more carefully in a bit. Also, full credit goes to @rudyhuyn for creating this. With his permission, I merely volunteered to bring this to the toolkit.

I'll work on the other pr requirements over the next few days.

[michael-hawker]

Cool, if it's @rudyhuyn's code, we should just get his buy-in on the PR even if it's just a comment saying he approves, it helps if he's signed the .NET Foundation CLA as well from the link above in the bot message.

[michael-hawker]

FYI there's also one in the platform samples apparently: https://github.com/microsoft/Windows-universal-samples/blob/master/Samples/XamlStateTriggers/cs/CustomTriggers/ControlSizeTrigger.cs

(Thanks @Arlodotexe for the link).

Do we want to worry about height too? Should it be combined or independent? Independent means you could have both triggers for the same state more easily, right?

[michael-hawker]

@dpaulino thoughts on the height? We also need docs and a unit test would probably be good?

[michael-hawker]

@dpaulino did you want to compare the implementations still and work on a quick test and sample for this still?

[dpaulino]

@michael-hawker hey sorry for the radio silence! I think having a separate controlHeightTrigger class may be preferred. Might be clearer in usage to see separate triggers for width and for height. Thoughts?

I'll try to make a sample for this soon, and yeah I can play around with the one from the official samples. If they perform similarly, what do you think is the best approach? Just c/p that code instead of using this one?

[michael-hawker]

@dpaulino having them combined as a single trigger I think would be preferable; as if they're separate triggers, you can only do OR for width/height. By having them combined, you have use && in the trigger logic if the dev sets a width and height. Then if they want an OR they just setup two separate copies of the trigger (one with Width and one with Height). See doc for StateTriggers as it'll apply is any trigger is matched.

I like that yours has the min/max range style as well, so maybe a combination of both?

As for adding a sample, we have a wiki doc here, but there's a ton of existing StateTrigger samples, so you can just copy one of those too as a starting point.

[ghost]

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

[dpaulino]

Oops sorry if I changed the reviewer! Not quite sure what happened!

[michael-hawker]

@dpaulino missing the header on the new test file 😉

[dotMorten]

Would it make more sense to make the default values NaN ?
It seems to me you need to set at least two parameters, but if all I want to do is trigger it when larger than a value (ie MinWidth/Height), I still need to set the Max values too. NaN could indicate "don't check".
Ie if I just want it to turn on when MinWidth > 300, I still have to set MaxWidth to Infinity or something else very large, or the expression would be false still.

[dpaulino]

Would it make more sense to make the default values NaN ?
It seems to me you need to set at least two parameters, but if all I want to do is trigger it when larger than a value (ie MinWidth/Height), I still need to set the Max values too. NaN could indicate "don't check".
Ie if I just want it to turn on when MinWidth > 300, I still have to set MaxWidth to Infinity or something else very large, or the expression would be false still.

Yeah that makes sense. Let me make some changes

[dpaulino]

@dotMorten the trigger now has double.NaN as default. Tests added as well to test the different scenarios. Thanks for the feedback.

[michael-hawker]

From @dotMorten:

Ie if I just want it to turn on when MinWidth > 300, I still have to set MaxWidth to Infinity or something else very large, or the expression would be false still.

@dpaulino wasn't this the case already as the default was Infinity? I'm confused. You should just be able to set MaxWidth="300" and be done with the original code, right?

We're concerned about the extra complexity in all the extra logic when it should have just worked as is?

Also tests are failing in the CI:

 Failed ControlSizeTriggerTest [587 ms]
  Failed ControlSizeTriggerTest (450,450,True) [584 ms]
  Error Message:
   Assert.AreEqual failed. Expected:<True>. Actual:<False>. 
  Stack Trace:
     at UnitTests.UWP.UI.Triggers.Test_ControlSizeTrigger.<>c__DisplayClass0_0.<ControlSizeTriggerTest>b__0() + 0x11c
   at System.Action.Invoke() + 0x1d
   at Microsoft.Toolkit.Uwp.DispatcherQueueExtensions.<>c__DisplayClass1_0.<EnqueueAsync>b__1() + 0x20
--- End of stack trace from previous location where exception was thrown ---

[dotMorten]

@michael-hawker UGH I totally misread that. I could swear the default was 0.0 for the max values, but I know see I must have had way too tired eyes to do a code review 🤦‍♂️. Getting lasik next week. Perhaps that'll help...

[dpaulino]

Ok sounds good, I'll revert the change and fix the unit tests.

[dpaulino]

I reverted the change and fixed the unit tests.

[dpaulino]

@michael-hawker The unit tests are passing for me locally, but the CI is failing. Is there something else I'm missing?

[dpaulino]

CI build test passing, yay!

[dpaulino]

Anything else I can do?

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.