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

AdvancedCollectionView's Source as DependencyProperty #1177

Closed
ghost opened this issue May 23, 2017 · 20 comments
Closed

AdvancedCollectionView's Source as DependencyProperty #1177

ghost opened this issue May 23, 2017 · 20 comments
Labels
helpers ✋ question ❔ Issues or PR require more information

Comments

@ghost
Copy link

ghost commented May 23, 2017

Is it possible to change AdvancedCollectionView's Source property into a DependencyProperty? To be able to use it in XAML markup. I want to instantiate ACV in XAML to enforce MVVM. I have all my ViewModels in a separate library.

@ghost ghost changed the title AdvancedCollectionView's Source as DependendyProperty AdvancedCollectionView's Source as DependencyProperty May 23, 2017
@nmetulev
Copy link
Contributor

have you tried to see if CollectionViewSource would work?

@tomzorz might have more ideas there too.

@nmetulev nmetulev added helpers ✋ question ❔ Issues or PR require more information labels May 23, 2017
@ghost
Copy link
Author

ghost commented May 24, 2017

@nmetulev what do you mean? I am using AdvancedCollectionView and not CollectionViewSource because of the filtering, sorting capabilities. But if what you are saying is using a CollectionViewSource as the Source of the AdvancedCollectionView, I haven't tried that. I will try that later and update what happens.

One thing I also notice just now, putting an AdvancedCollectionView in the Page.Resources section with no properties set except its key results to a squiggly line. It says, it is not an IList. I think, it is not just the Source property that stops me from using it in XAML.

@tomzorz
Copy link
Contributor

tomzorz commented May 24, 2017

@Rivolvan it wasn't meant to be used in Xaml (despite the naming), because declaring filters/sorts made more sense in code (at least for me, but again I didn't see anyone object).

I read a bunch of SO questions and articles, and most of the Xaml solutions ended up with just stuffing hacky codebehind code in a behavior and using that on the CollectionView. I didn't see how that was better than having it in the ViewModel.

@tomzorz
Copy link
Contributor

tomzorz commented May 24, 2017

The docs give an example of the preferred usage http://docs.uwpcommunitytoolkit.com/en/master/helpers/AdvancedCollectionView/

@ghost
Copy link
Author

ghost commented May 25, 2017

As I see it, AdvancedCollectionView is made to be UWP specific. And when making cross platform apps, ViewModels are treated as platform agnostic. So it makes sense for those developers and me to put it in the XAML of the UWP app and put the filter logic (usually by a command) in the ViewModel. That way, the filter logic can be shared with other platforms no matter what kind of "CollectionView" is used by those platforms. As for XAML declaration, it seems easier to put it this way:

<AdvancedCollectionView Source={x:Bind ViewModel.Items, Mode=OneWay}> <interactivity:Interaction.Behaviors> <core:EventTriggerBehavior EventName="Filter"> <core:InvokeCommandAction Command="{x:Bind ViewModel.Filter}" InputConverter="{StaticResource FilterConverter}" </core:EventTriggerBehavior> </interactivity:Interaction.Behaviors>

@tomzorz
Copy link
Contributor

tomzorz commented May 25, 2017

@Rivolvan I see your point, and I do have to mention that during implementation I did not consider cross-platform usage. I've seen a hole in UWP that I wanted to fix :)

As for the actual crossplat usage: I'd still prefer if this this wouldn't be in a "uwp toolkit" but maybe in a crossplat toolkit, allowing you not to put this in XAML. But as this thing is in a "uwp toolkit" probably the only solution aside from grabbing the source is making it work in XAML.

To that end I see the following immediate tasks:

  • make AdvancedCollectionView a descendant of DependencyObject
  • make the Source property a DependencyProperty
  • make SortDescription a descendant of DependencyObject
  • fix the constructors so they work in XAML

This would probably enable the usage you want. In the future maybe we could change other properties into DPs, so they can be used as an updating binding source / target.

@tomzorz
Copy link
Contributor

tomzorz commented May 25, 2017

Two more things:

  • I'd like to see a little input from the repo owners / code reviewers to see what they think of this change before jumping into implementation.
  • While I'd be happy to work on this, as far as I see I don't have time to do that for the next 4-5 weeks.

@ghost
Copy link
Author

ghost commented May 25, 2017

@tomzorz don't get me wrong, I am very thankful that you have taken the time to fill up a hole in UWP that every business app developer needs. And know that I am not saying that you have done this wrong. I just want to know the design behind the implementation. And I wanted to know if there is a workaround to use it in XAML.

I am also in the middle of a project right now, so as a hack, I just put the AdvancedCollectionView in the code behind and talk with the ViewModel via MVVMLight's Messenger. As I said, my ViewModels are in another library so I guess this is the best way I can do for now.

@ghost
Copy link
Author

ghost commented May 25, 2017

If the creators of this toolkit however sees that we can make the changes you said, I'll be happy to help or maybe make a PR when I can make time.

Thanks again!

@ghost
Copy link
Author

ghost commented May 25, 2017

As an aside, I think we can never make AdvancedCollectionView platform agnostic however. It relies on UWP APIs that is needed for it to work in UWP's List controls. As I said earlier, the best way we can do for now is make it compatible with XAML usage.

@nmetulev
Copy link
Contributor

Do you see this creating any breaking changes for AdvancedCollectionView?

@tomzorz
Copy link
Contributor

tomzorz commented May 26, 2017

@nmetulev I don't see any at the moment - maybe just the fact that when it becomes a DependencyObject you'll really avoid placing it in the VM.

Maybe we should create a proxy class that lives in XAML? Best of both worlds?

@ghost
Copy link
Author

ghost commented May 26, 2017

In its current state, the developers who use AdvancedCollectionView in their ViewModels now will still use it there even when we change it to a DependencyObject because, more than likely, their ViewModels are in the same project as their Views or in a UWP Library. (This toolkit can only be referenced from a UWP library anyway)

I don't see any breaking changes when we make this, it will only make it XAML friendly, so win-win.

@skendrot
Copy link
Contributor

Making it a DP does not remove the ability to also create it in code.

@tomzorz
Copy link
Contributor

tomzorz commented May 26, 2017

@skendrot I never denied that ;) It just feels a wee bit weird (at least for me)

@nmetulev
Copy link
Contributor

As long as there are no breaking changes and it can still be used the way it is used right now, I'm ok with you extending it so it can be used in more scenarios. Feel free to submit a PR. :)

@nmetulev
Copy link
Contributor

Up?

@windowstoolkitbot
Copy link

This issue seems inactive. It will automatically be closed in 14 days if there is no activity.

@windowstoolkitbot
Copy link

This issue seems inactive. It will automatically be closed in 7 days if there is no activity.

@windowstoolkitbot
Copy link

Issue is inactive. It was automatically closed.

@CommunityToolkit CommunityToolkit locked as resolved and limited conversation to collaborators Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
helpers ✋ question ❔ Issues or PR require more information
Projects
None yet
Development

No branches or pull requests

4 participants