Making the ListExtensions feature in-line with the documentation #3209
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…de is consistent with the docs
|
Thanks deanchalk 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
assigned 
36 tasks
michael-hawker
requested changes
Apr 23, 2020
There was a problem hiding this comment.
Some hopefully minor updates needed. We should also make sure we update the doc, as it also still calls out these extensions as being something like a subclass of ListViewBase (which I think was how they worked in the past until we moved them to be attached properties).
Thanks!
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ListViewExtensions/ListViewExtensionsPage.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/ListViewBase/ListViewExtensions.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/ListViewBase/ListViewExtensions.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ListViewExtensions/ListViewExtensionsPage.xaml.cs
Outdated
Show resolved
Hide resolved
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
|
@michael-hawker - I have completed the above tasks and added to my branch. Let me know if theres anything else to do :) |
Kyaa-dost
approved these changes
May 18, 2020
michael-hawker
approved these changes
May 28, 2020
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ListViewExtensions/ListViewExtensionsPage.xaml.cs
Outdated
Show resolved
Hide resolved
…/ListViewExtensionsPage.xaml.cs Tested removing this line, as duplicated logic, done through binding.
Kyaa-dost
added
feature 💡
documentation 📃
improvements ✨
and removed
auto merge ⚡
feature 💡
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3078
According to the documentation - https://docs.microsoft.com/en-us/windows/communitytoolkit/extensions/listviewextensions - the
ListViewExtensionsset of attached properties should include a 'Command' attached property. According to the git history this was never part of the code, which is unusual.Anyway, I have added all of the necessary code to support this documented feature, and updated the sample app page accordingly.
PR Type
What kind of change does this PR introduce?
Bugfix
Feature
Sample app changes
What is the current behavior?
Currently there is no Command attached property fir ListExtensions, despite there being official docs for it
What is the new behavior?
The missing Command attached property has been added, and the sample page has been updated
PR Checklist
Please check if your PR fulfills the following requirements:
Other information