Skip to content

Conversation

JohnnyWestlake
Copy link
Contributor

Currently, animation behaviours set translation offsets of 1, rotation angle of 1, and blur radius of 1 by default. This changes them all to be 0 which most users would expect.

@msftclas
Copy link

Hi @JohnnyWestlake, 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;

@deltakosh
Copy link
Contributor

Unfortunately this would be a breaking change.
@pedrolamas: CI failed weirdly on this one

@JohnnyWestlake
Copy link
Contributor Author

JohnnyWestlake commented Aug 20, 2016

Would it be breaking? I'm not sure any users would be expecting to attach the behaviour with no properties set and have it automatically just translate their element by a single pixel / rotate by a single pixel. I'd wager no one is relying on this single pixel change. Feels wrong as it is. If anyone actually wants it to do anything right off the bat they'd set the values on it surely?

@deltakosh
Copy link
Contributor

Yeah I think you're right:)

I won't block the PR but just wanting other thoughts

@dotMorten
Copy link
Contributor

It's definitely a change in behavior. Unless we define the current behavior as a bug. The above argument that no one would be using those defaults as they make no sense and hardly does anything are more or less valid.

@pedrolamas
Copy link
Member

This is a breaking change in the current behavior, but frankly, I'd expect the default values to be 0... maybe it's enough to get this into the next minor version change and ensure we mention it on the change log?

CI failed weirdly on this one

@deltakosh seems this was caused by some issue with GitVersion, but I'm not expert on it, @onovotny is our man!

If we don't know what caused it or can't fix it, my advice would be to just use the AppVeyor build number like x.y.z.buildNumber

@ScottIsAFool
Copy link
Contributor

I agree that the current values are probably incorrect, and so LGTM :) As @pedrolamas says, we probably just need to make a note of the change in the release notes.

@pedrolamas
Copy link
Member

@JohnnyWestlake can you please just update this PR by merging the latest from dev branch into it? This is just so that the build gets fixed! :)

@deltakosh
Copy link
Contributor

Agree on validating the breaking changes. It is minor and I will document it.

@deltakosh
Copy link
Contributor

LGTM

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

Successfully merging this pull request may close these issues.

6 participants