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.

Sign up for GitHub

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

Jump to bottom

Remove Animations Package reference from Controls package #3578

Closed
michael-hawker opened this issue Nov 24, 2020 · 5 comments · Fixed by #3611
Closed

Remove Animations Package reference from Controls package #3578

michael-hawker opened this issue Nov 24, 2020 · 5 comments · Fixed by #3611
Labels
animations 🏮 controls 🎛️ help wanted Issues identified as good community contribution opportunities improvements ✨ in progress 🚧
Milestone
7.0

Comments

@michael-hawker
Copy link
Member

michael-hawker commented Nov 24, 2020 •
edited
Loading

Describe the bug

The Animations package is currently being pulled in as a dependency for the Controls package. However, there's only two small uses of it in the package: RangeSelector for implicit animations, and ScrollHeader for the QuickReturnHeaderBehavior.

Expected behavior

I think removing the animation for the Tooltip in RangeSelector should be simple enough. I'm not as sure what we do with ScrollHeader as a component.

But migrating these off of the animation package or moving the ScrollHeader to a different package would be a big win for our dependency graph.

This may be dependent on work we do to clean-up the Behaviors/Animations package in general with its reliance on Win2D as well.

Tied to #3062 and #3145
@michael-hawker michael-hawker added help wanted Issues identified as good community contribution opportunities controls 🎛️ animations 🏮 improvements ✨ labels Nov 24, 2020
@michael-hawker michael-hawker added this to the 7.0 milestone Nov 24, 2020
@skendrot
Copy link
Contributor

I think using the composition api directly would be better than bringing in the animation package

@michael-hawker michael-hawker mentioned this issue Dec 4, 2020
Closed
@Rosuavio
Copy link
Contributor

Rosuavio commented Dec 4, 2020 •
edited
Loading

I did some investigation on some of the interesting dependencies from the controls.

ScrollHeader

Microsoft.Xaml.Behaviors.Uwp.Managed

Microsoft.Xaml.Interactivity;Microsoft.Xaml.Interactivity.Interaction.GetBehaviors(DependencyObject obj)

Microsoft.Toolkit.Uwp.UI

Microsoft.Toolkit.Uwp.UI.Extensions.VisualTree.FindAscendant<T>(this DependencyObject element)

Microsoft.Toolkit.Uwp.UI.Animations

Microsoft.Toolkit.Uwp.UI.Animations.Behaviors.FadeHeaderBehavior
Microsoft.Toolkit.Uwp.UI.Animations.Behaviors.QuickReturnHeaderBehavior
Microsoft.Toolkit.Uwp.UI.Animations.Behaviors.StickyHeaderBehavior

TileControl

Microsoft.Toolkit.Uwp.UI

Microsoft.Toolkit.Uwp.UI.Extensions.VisualTree.FindDescendant<T>(this DependencyObject element)

Microsoft.Toolkit.Uwp.UI.Animations

Microsoft.Toolkit.Uwp.UI.Animations.Expressions.ExpressionNode
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.CompositionExtensions.GetReference(this Visual compObj)
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.CompositionExtensions.GetSpecializedReference<T>(this CompositionPropertySet ps)
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.ExpressionFunctions.Ceil(ScalarNode val)
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.ExpressionFunctions.Conditional(BooleanNode condition, ScalarNode trueCase, ScalarNode falseCase)
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.ExpressionFunctions.Abs(ScalarNode val)
Microsoft.Toolkit.Uwp.UI.Animations.Expressions.ManipulationPropertySetReferenceNode

Other

ImageCropper, Eyedropper, InfiniteCanvas all use a bunch of win2d.

@Rosuavio
Copy link
Contributor

Rosuavio commented Dec 8, 2020

#3611 should take care of RangeSelector

@Rosuavio Rosuavio linked a pull request Dec 8, 2020 that will close this issue
Remove animations from RangeSelector #3611
Merged
7 tasks
@Rosuavio Rosuavio removed a link to a pull request Dec 8, 2020
Remove animations from RangeSelector #3611
Merged
7 tasks
@michael-hawker michael-hawker linked a pull request Dec 8, 2020 that will close this issue
Remove animations from RangeSelector #3611
Merged
7 tasks
@Rosuavio Rosuavio mentioned this issue Dec 9, 2020
Closed
7 tasks
@Rosuavio
Copy link
Contributor

Rosuavio commented Dec 9, 2020

#3615 Might be hamfisted but its something to remove TileControl's and OrbitViews dep on animation.

@ghost ghost added the in progress 🚧 label Dec 11, 2020
@Rosuavio Rosuavio mentioned this issue Dec 14, 2020
Merged
7 tasks
ghost pushed a commit that referenced this issue Dec 15, 2020
Orbitview remove animation expressions (#3627)
a20e824 
<!-- 🚨 Please Do Not skip any instructions and information mentioned below as they are all required and essential to evaluate and test the PR. By fulfilling all the required information you will be able to reduce the volume of questions and most likely help merge the PR faster 🚨 -->

<!-- 📝 It is preferred if you keep the "☑️ Allow edits by maintainers" checked in the Pull Request Template as it increases collaboration with the Toolkit maintainers by permitting commits to your PR branch (only) created from your fork.  This can let us quickly make fixes for minor typos or forgotten StyleCop issues during review without needing to wait on you doing extra work. Let us help you help us! 🎉 -->

<!-- Add a brief overview here of the feature/bug & fix. -->
Sept towards #3578. 

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
<!-- - Feature -->
<!-- - Code style update (formatting) -->
- Refactoring (no functional changes, no api changes)
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
OrbitView uses the Toolkit Animation.Expressions API.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
OrbitView uses [ExpressionAnimation](https://docs.microsoft.com/en-us/uwp/api/windows.ui.composition.expressionanimation)

## PR Checklist

Please check if your PR fulfills the following requirements:

- [ ] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] Sample in sample app has been added / updated (for bug fixes / features)
    - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)
- [ ] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [ ] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [ ] Contains **NO** breaking changes

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. 
     Please note that breaking changes are likely to be rejected within minor release cycles or held until major versions. -->


## Other information
@Rosuavio Rosuavio mentioned this issue Dec 16, 2020
Merged
7 tasks
ghost pushed a commit that referenced this issue Dec 17, 2020
TileControl: Remove Animations.Expressions usage (#3633)
4a59111 
<!-- 🚨 Please Do Not skip any instructions and information mentioned below as they are all required and essential to evaluate and test the PR. By fulfilling all the required information you will be able to reduce the volume of questions and most likely help merge the PR faster 🚨 -->

<!-- 📝 It is preferred if you keep the "☑️ Allow edits by maintainers" checked in the Pull Request Template as it increases collaboration with the Toolkit maintainers by permitting commits to your PR branch (only) created from your fork.  This can let us quickly make fixes for minor typos or forgotten StyleCop issues during review without needing to wait on you doing extra work. Let us help you help us! 🎉 -->

<!-- Add a brief overview here of the feature/bug & fix. -->
Sept towards #3578.

Remove the `TileControl`'s usage of the `Animations.Expressions` ns and thus the animations package.

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
<!-- - Feature -->
<!-- - Code style update (formatting) -->
- Refactoring (no functional changes, no api changes)
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
`TileControl`'s uses `Animations.Expressions` to build expression animation.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
`TileControl`'s uses strings to build expression animation.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [ ] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] Sample in sample app has been added / updated (for bug fixes / features)
    - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)
- [ ] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [ ] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [ ] Contains **NO** breaking changes

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. 
     Please note that breaking changes are likely to be rejected within minor release cycles or held until major versions. -->


## Other information
@michael-hawker
Copy link
Member Author

This work has been completed.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
animations 🏮 controls 🎛️ help wanted Issues identified as good community contribution opportunities improvements ✨ in progress 🚧
Projects
None yet
Milestone
7.0
Development

Successfully merging a pull request may close this issue.

Remove animations from RangeSelector
3 participants
@skendrot @Rosuavio @michael-hawker