Skip to content

Conversation

pekspro
Copy link
Contributor

@pekspro pekspro commented Sep 21, 2016

No description provided.

@msftclas
Copy link

Hi @pekspro, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -0,0 +1,18 @@
namespace Microsoft.Toolkit.Uwp.UI.Controls
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the copyright info on top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I missed that! Now that’s fixed.

@nmetulev
Copy link
Contributor

Should we also have status for canceled and completed swipe? I think that would also be helpful

@nmetulev nmetulev self-assigned this Sep 21, 2016
@pekspro
Copy link
Contributor Author

pekspro commented Sep 22, 2016

This is absolutely worth to discuss :-). Currently there are these states:

Idle
Starting

DisabledSwipingToLeft
SwipingToLeftThreshold
SwipingPassedLeftThreshold

DisabledSwipingToRight
SwipingToRightThreshold
SwipingPassedRightThreshold

When you SwipeStatusChanged event is triggered you get both the new and old state. If you have completed a swipe new value will be Idle and old value will be SwipingPassedLeft/RightThreshold. So I don't see any need for a Completed/Cancelled status - but it would for sure be easier to check this so I have no problems adding it :-)

If this is added should the status be changed to Idle immediately after the status has been Completed? Or should the Status just be Completed until a new swipe starts?

I have really no opinions about this. What do you think? Any thoughts from @deltakosh or @Librazy or someone else?

@deltakosh
Copy link
Contributor

I have no problem with current implementation.
Once we agreed upon what to do, do you mind updating the documentation with all these cool new features?

@Librazy
Copy link

Librazy commented Sep 23, 2016

The SwipeStatusChanged event is an brilliant idea when we need to maintain the status (especially when something depends on the transfer of SwipeStatus). But when we just care about some special status transitions like starting/ canceling and completing swipe, SwipeStatusChanged seems to heavy. And we must check the SwipeStatusChangedEventArgs to find out whether the event is our concern.
IMO, we need some light weight events or commands to represent some import status transitions.

SwipeStatusChanged event is a must-have, but not enough.

@nmetulev
Copy link
Contributor

@pekspro that makes sense what you describe, I'm ok with the current implementation as long as we have it documented on how to detect completed and canceled. Once documentation is done, it's good to go.

@Librazy not sure if having standalone events provide additional value over this one event. Firing multiple events at the same time seems more heavy that having one event. @deltakosh, any thoughts on this?

@pekspro
Copy link
Contributor Author

pekspro commented Sep 23, 2016

@nmetulev, @deltakosh I will start the with documentation very soon, or maybe tomorrow :-). This is absolutely essential I think.

When it comes to additional events I just want to notify that there already is Left/RightCommandRequested event which covers the Completed event. Detecting starting will be trivial with the SwipeStatusEvent. Cancelling will also be possible but slightly harder. I will cover all these cases in the documentation. That said I’m not all against new events :-).

@deltakosh
Copy link
Contributor

i agree with Nikola, we should start with only changed event and add more if needed
The Left/Right are more related to commanding to me.

@deltakosh deltakosh merged commit d650946 into CommunityToolkit:dev Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants