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

Initial work on adding remaining animations #1232

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bijington
Copy link
Contributor

@bijington bijington commented Jun 11, 2023

Description of Change

Linked Issues

PR Checklist

Additional information

await animation.Animate(label);
stopwatch.Stop();

stopwatch.ElapsedMilliseconds.Should().BeCloseTo(animation.Length, 50);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we get this value from some constant in Animation from library code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep happy to stick that somewhere more central

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we have different default duration for each animation, should this value be the same as the animation duration we pass to BaseAnimation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might highlight that the code doesn't read well. the Length property of the BaseAnimation is that value we pass into the constructor. The 50 is my attempt at verifying that the completed duration was within 50 milliseconds of the expected duration. Perhaps this could be a calculated percentage instead. I still need to find a reliable way of verifying this because running the tests locally don't yield positive test results for all yet.

@ghost ghost added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Aug 2, 2023
@VladislavAntonyuk
Copy link
Collaborator

Will this PR resolve the animation doubled duration issue? #1231

@bijington
Copy link
Contributor Author

Yes it will

@bijington
Copy link
Contributor Author

I believe I have already addressed that issue in this PR I'm just struggling to get the unit test to prove it

{
ArgumentNullException.ThrowIfNull(view);

var duration = (uint)(Length / Math.Ceiling(StartFactor / ReducingAmount)) / 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must be missing something obvious but I can't work out why this fails when being tested. The current values mean there are 3 shakes crossing the x == 0 point twice. This should result in 6 awaits however the test results in a failure because the animation completes within roughly 200ms. Given the above I calculate with a Length == 300, duration should equal 50 and therefore result in a passing test. However it doesn't appear to work

@bijington
Copy link
Contributor Author

As far as I can tell it might be down to our use of Task.Delay in the mocking of the animation layer. I have been trying to investigate with a more accurate option for mocking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants