CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.89k stars 1.38k forks source link

Carousel ItemClick #1159

Closed TheSETJ closed 4 years ago

TheSETJ commented 7 years ago

Hello again.

I think adding an ItemClick event handler to Carousel is a good idea.

skendrot commented 7 years ago

Closing as a duplicate of #1134. No need for both events

JohnnyWestlake commented 7 years ago

Are these not different use cases though? Selection changed will fire just when browsing through the carousel rather than explicitly clicking on the current item

skendrot commented 7 years ago

Reoping to allow for discussion

skendrot commented 7 years ago

I'm personally not a fan of the ItemClick model from ListView. One, you have to enable it. Two, it's no different from adding a Tapped event on your ItemTemplate

Mimetis commented 7 years ago

Interesting question.

To be honest, I have already asked myself this question.. Like the SelectionChanged event, the ItemClick event could eventually be a nice feature if we think it's something developers needs on the most of the cases. Actually (but i may be wrong) I don't think it's a "most of the cases" usage.

For the time being, you can add the item click directly in your item Template (as well as any animation on your item)

Is that make sense for you ?

TheSETJ commented 7 years ago

@Mimetis Actually, with discussions that happened here, It makes sense. I think @skendrot is right about no difference between ItemClick and Tapped event for ItemTemplate. At least in all of the cases, I worked on, it's true. But I wonder why there should be 2 event handler with same exact functionality if there is no difference?

JohnnyWestlake commented 7 years ago

For ItemClick it has the option of being controlled by it's sibling property IsItemClickEnabled, and in case when ItemTemplateSelectors or shared DataTemplates are used (the most important case imo), doesn't require Tapped events on each one or creating a local copy of the template just for the Tapped event.

So in my mind that leaves the question of how often people will be sharing the same DataTemplate between different carousels on different pages, which didn't seem like that much of an obscure pattern

That being said it's only a small developer convenience, and also it's closest analogue control FlipView didn't have such an event. But I don't think it's a terrible idea

nmetulev commented 7 years ago

ItemClick even is very helpful when it comes to supporting multiple input modalities. You can use the keyboard or controller to select items, but without the ItemClick event, you need to add support for keyboard press, tapped, and other potential input modalities. ItemClick and IsItemClickEnabled allows the control to handle all that for you.

skendrot commented 7 years ago

If we add the property and event, should the control just move to be a ListViewBase control? Why duplicate all of the properties?

Mimetis commented 7 years ago

Actually, I'm trying to move to ListViewBase. The problem i'm facing right now is I have to override all the ListViewBase properties and event, because the items selection and so on are really different from the base listview. I will update this thread once I will be sure migrating to ListViewBase is making sense.

JohnnyWestlake commented 7 years ago

Migrating will add a whole bunch of useless properties around things like SelectionMode, MultiSelect, Footer, Header, Group support (perhaps not useless, but not accounted for), CanDragItems, Reorder Modes and a whole bunch of others - and ones that do have some edge uses (like Incremental Loading, ISemanticZoom etc) need to be properly handled.

I'd vote it staying as an ItemsControl just to avoid any developer confusion as to why there are all these properties they can't / shouldn't use.

EDIT: Though I do think moving it to Selector as @skendrot says in #1134 seems like the right idea.

Mimetis commented 7 years ago

@JohnnyWestlake : Couldn't say it better :) For the next version, moving to Selector should be a reasonable solution

skendrot commented 7 years ago

It's not possible to move to Selector. It has an internal constructor :(

nmetulev commented 7 years ago

Opening this up for anyone that might want to do it.

gbarcho commented 6 years ago

Whats the current status?

Kyaa-dost commented 4 years ago

@TheSETJ any update on this?

TheSETJ commented 4 years ago

@Kyaa-dost It's a long time I didn't work on any UWP project, thus I don't know.

Kyaa-dost commented 4 years ago

Closing this issue for now as we don't seem to be clear on this one.