Skip to content
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

New behavior KeyDownTriggerBehavior #4160

Merged

Conversation

ArchieCoder
Copy link
Contributor

@ArchieCoder ArchieCoder commented Aug 4, 2021

Fixes #4152

Discussion: #4152

Add a new behavior that listens to a key press event on the associated UIElement and triggers the set of actions.

PR Type

What kind of change does this PR introduce?

New behavior

What is the current behavior?

What is the new behavior?

New behavior is added.

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:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • 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
Copy link

ghost commented Aug 4, 2021

Thanks ArchieCoder 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 🙌

@ghost ghost requested review from michael-hawker and azchohfi August 4, 2021 18:54
@net-foundation-cla
Copy link

net-foundation-cla bot commented Aug 4, 2021

CLA assistant check
All CLA requirements met.

@net-foundation-cla
Copy link

CLA assistant check
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.

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

@ghost ghost requested a review from Rosuavio August 4, 2021 18:54
ArchieCoder and others added 2 commits August 5, 2021 13:26
…vior.cs

Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
…vior.cs

Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
@michael-hawker
Copy link
Member

@ArchieCoder did you forget to push? Looks like you replied to some comments, but I don't see the changes up here in the PR.

ArchieCoder and others added 2 commits August 6, 2021 09:02
Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
@ArchieCoder
Copy link
Contributor Author

@michael-hawker One last thing, is the doc generated after the PR is merged?

@michael-hawker
Copy link
Member

@ArchieCoder you need to manually submit a doc PR, though I don't think we have a base Behavior page in the docs, as we'd probably just want to point folks to general overview since they're pretty simple (vs. having a dedicated page for each one). @XAML-Knight this could be a good thing we could have you do to get familiar with the docs and the other behaviors.

The API doc is auto-generated when we do the release as long as all the XML comments are there, which we enforce with Style-cop.

Looks like the build is failing as the header is still missing on the new file. You can also run UpdateHeaders.bat or '.\build.ps1 -Target UpdateHeaders' to add it.

@ArchieCoder
Copy link
Contributor Author

@michael-hawker I ran the tool and submitted the header.

I was just wondering about the doc because for example the other behavior has this file and it is nowhere in the solution. This is why I supposed it is generated with another system.

"DocumentationUrl": "https://raw.githubusercontent.com/MicrosoftDocs/WindowsCommunityToolkitDocs/master/docs/behaviors/FocusBehaviors.md

@XAML-Knight
Copy link
Contributor

XAML-Knight commented Aug 6, 2021

Hi @ArchieCoder , I've started a dev branch for this behavior's documentation; feel free to fork & embellish:

https://github.com/XAML-Knight/WindowsCommunityToolkitDocs/tree/dev/KeyDownTriggerBehavior

@ArchieCoder
Copy link
Contributor Author

@michael-hawker Hi, is there any other steps that I need to do?

@michael-hawker
Copy link
Member

@ArchieCoder the CI is still failing on a StyleCop issue:

D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Behaviors\Keyboard\KeyDownTriggerBehavior.cs(17,5): error SA1505: An opening brace should not be followed by a blank line. [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Behaviors\Microsoft.Toolkit.Uwp.UI.Behaviors.csproj]

Other than that though, I think it's all set to be tested and reviewed, so we'd just need to get sign-off from a couple of folks. I'll take another pass now, but I know it's been looking pretty good. Appreciate all your work on this! 🦙❤

@ArchieCoder
Copy link
Contributor Author

Ok, I didn't notice the StyleCop. Fixed now.

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Code-side looks good to me. @XAML-Knight or @RosarioPulella feel like taking it for a spin? 🙂

@@ -1475,6 +1476,7 @@
<Name>Visual C++ 2015 Runtime for Universal Windows Platform Apps</Name>
</SDKReference>
</ItemGroup>
<ItemGroup />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<ItemGroup />

We can remove this to minimize noise.

"CodeUrl": "https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI.Behaviors/Keyboard/KeyDownTriggerBehavior.cs",
"XamlCodeFile": "/SamplePages/KeyDownTriggerBehavior/KeyDownTriggerBehaviorXaml.bind",
"Icon": "/Assets/Helpers.png",
"DocumentationUrl": "https://raw.githubusercontent.com/MicrosoftDocs/WindowsCommunityToolkitDocs/master/docs/behaviors/KeyboardBehaviors.md"
Copy link
Contributor

@XAML-Knight XAML-Knight Aug 10, 2021

Choose a reason for hiding this comment

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

Make sure this DocumentationUrl property points to the correct markdown file, for the corresponding file in the
WindowsCommunityToolkitDocs repo. A markdown file doesn't exist in the Docs repo for this Keyboard behavior yet, of course, so we'll need to add it, as an additional PR to the Docs repo.

  • @ArchieCoder are you planning on submitting a corresponding PR to the WindowsCommunityToolkitDocs repo, for the markdown file?
    • Feel free to reach out regarding how to submit a PR to the Docs repo
    • More online guidance for submitting a Docs PR guidance here and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XAML-Knight can I just remove the DocumentationUrl? This behavior is so simple that the existing comments in the code is self-explanatory.

So far, it took me like 20 mins to do the behavior, but all the rest is many hours!

Copy link
Contributor

@XAML-Knight XAML-Knight left a comment

Choose a reason for hiding this comment

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

Approved. Pushed commit eab041e to this user branch, for modification of samples.json, to fit more descriptive markdown file name in the Docs repo.

Submitted related Docs PR #556

"CodeUrl": "https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI.Behaviors/Keyboard/KeyDownTriggerBehavior.cs",
"XamlCodeFile": "/SamplePages/KeyDownTriggerBehavior/KeyDownTriggerBehaviorXaml.bind",
"Icon": "/Assets/Helpers.png",
"DocumentationUrl": "https://raw.githubusercontent.com/MicrosoftDocs/WindowsCommunityToolkitDocs/master/docs/behaviors/KeyDownTriggerBehavior.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

DocumentationUrl modified, to fit more descriptive markdown file name

@michael-hawker
Copy link
Member

Thanks @ArchieCoder for this contribution to the Toolkit! 🎉🎉🎉

@michael-hawker michael-hawker merged commit bd71319 into CommunityToolkit:main Aug 13, 2021
@ArchieCoder
Copy link
Contributor Author

Thanks you for your help @michael-hawker !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants