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

StateContainer, Add ChangeStateWithAnimation() #1021

Merged
merged 11 commits into from
Feb 27, 2023
Merged

Conversation

VladislavAntonyuk
Copy link
Collaborator

@VladislavAntonyuk VladislavAntonyuk commented Feb 22, 2023

Description of Change

Instead of throwing exceptions, just return. The animation operation is canceled but we still can continue going to the next state.

942

Linked Issues

PR Checklist

Additional information

@VladislavAntonyuk VladislavAntonyuk self-assigned this Feb 22, 2023
@brminnick
Copy link
Collaborator

Hey Vlad! The experience described in the Issue is the expected behavior.

We added CanStateChange and documented how to use it in the docs to avoid this Exception:

https://learn.microsoft.com/dotnet/communitytoolkit/maui/layouts/statecontainer#viewmodel

@brminnick
Copy link
Collaborator

brminnick commented Feb 22, 2023

The fix for #942 should be specific to resizing the Window on WinUI and not remove the StateContainerException in its entirety.

@brminnick brminnick closed this Feb 22, 2023
@VladislavAntonyuk VladislavAntonyuk changed the title Fixes StateContainer crash app when state change too fast #942 StateContainer new API Feb 24, 2023
@brminnick brminnick linked an issue Feb 24, 2023 that may be closed by this pull request
8 tasks
@brminnick brminnick changed the title StateContainer new API StateContainer, Add ChangeStateWithAnimation() Feb 24, 2023
brminnick
brminnick previously approved these changes Feb 26, 2023
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Thanks Vlad!

I added some tests to ensure/verify that the user can't change CurrentState while the animations are running.

Check out the code I added to ensure it looks good.

I'll approve this, and if you think it looks good, you can go ahead and merge it 🙌

@brminnick brminnick added the pending documentation This feature requires documentation label Feb 26, 2023
@brminnick
Copy link
Collaborator

brminnick commented Feb 26, 2023

@bijington Are you able to help update the docs for this PR? I've added do not merge Do not merge this PR and pending documentation This feature requires documentation for now to ensure we update the docs.

Removed APIs

StateContainer.ShouldAnimateOnStateChangeProperty

Added APIs

/// <summary>
/// Change state with custom animation.
/// </summary>
public static async Task ChangeStateWithAnimation(
 		BindableObject bindable,
 		string? state,
 		Animation? beforeStateChange,
 		Animation? afterStateChange,
 		CancellationToken token);

/// <summary>
/// Change state with custom animation.
/// </summary>
public static async Task ChangeStateWithAnimation(
 		BindableObject bindable,
 		string? state,
 		Func<VisualElement, CancellationToken, Task>? beforeStateChange,
 		Func<VisualElement, CancellationToken, Task>? afterStateChange,
 		CancellationToken cancellationToken);

/// <summary>
/// Change state using the default fade animation.
/// </summary>
public static async Task ChangeStateWithAnimation(BindableObject bindable, string? state, CancellationToken token);

@brminnick
Copy link
Collaborator

Removed APIs

^ FYI @jfversluis This is a breaking change that we'll need to note in the release notes for v5.0.0

@brminnick brminnick added do not merge Do not merge this PR approved This Proposal has been approved and is ready to be added to the Toolkit labels Feb 26, 2023
@VladislavAntonyuk
Copy link
Collaborator Author

VladislavAntonyuk commented Feb 26, 2023

@brminnick brminnick removed pending documentation This feature requires documentation do not merge Do not merge this PR labels Feb 27, 2023
@brminnick brminnick enabled auto-merge (squash) February 27, 2023 17:08
@brminnick brminnick merged commit 5470ec7 into main Feb 27, 2023
@brminnick brminnick deleted the 942-state-conrainer-fix branch February 27, 2023 17:29
@Strypper
Copy link

Thanks guys really appreciate your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This Proposal has been approved and is ready to be added to the Toolkit
Projects
None yet
3 participants