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

fix: ensure attribute animations respond to code changes #1452

Merged
merged 6 commits into from
Aug 9, 2024

Conversation

eriklimakc
Copy link
Contributor

@eriklimakc eriklimakc commented Jul 30, 2024

GitHub Issue: #1451

PR Type

What kind of change does this PR introduce?

  • Bugfix

Description

When TextBox was disabled PlaceholderText and Header were not properly placed.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@eriklimakc eriklimakc requested a review from kazo0 July 30, 2024 15:43
@eriklimakc eriklimakc self-assigned this Jul 30, 2024
@eriklimakc eriklimakc linked an issue Jul 30, 2024 that may be closed by this pull request
5 tasks
@eriklimakc eriklimakc force-pushed the dev/ERLI/1451-textbox-disabled branch from 7908787 to 761045d Compare July 30, 2024 16:23
@kazo0
Copy link
Collaborator

kazo0 commented Aug 1, 2024

@Mergifyio backport release/stable/5.1

Copy link

mergify bot commented Aug 1, 2024

backport release/stable/5.1

✅ Backports have been created

@eriklimakc eriklimakc requested a review from kazo0 August 1, 2024 18:11
@eriklimakc eriklimakc marked this pull request as draft August 2, 2024 15:47
@eriklimakc
Copy link
Contributor Author

eriklimakc commented Aug 2, 2024

To have animations taking action also when Text, PlaceholderText, Header are changed via code, we need to use those VisualStateGroups.

I tried to cleanup and avoid repetition as much as I could. From my tests everything is working everywhere, except the animation of Header upwards when text is entered. On the video below the Filled TextBox is using only Setters while the Outlined is using DoubleAnimation to animate the Header upwards:

wasm.animation.mp4

I left comments on the code:

<!-- Setters instead of animations on WASM work -->
<Setter Target="HeaderElement_CompositeTransform.ScaleX" Value="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformScales}, TargetNullValue=1, FallbackValue=1}" />
<Setter Target="HeaderElement_CompositeTransform.ScaleY" Value="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformScales}, TargetNullValue=1, FallbackValue=1}" />
<Setter Target="HeaderElement_CompositeTransform.TranslateY" Value="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformTranslateY}, TargetNullValue=0, FallbackValue=0}" />
</VisualState.Setters>
<!-- Animations now working properly on WASM -->
<!--<Storyboard>
<DoubleAnimation Storyboard.TargetName="HeaderElement_CompositeTransform"
Storyboard.TargetProperty="TranslateY"
Duration="{StaticResource MaterialTextBoxAnimationDuration}"
EasingFunction="{StaticResource MaterialEaseInOutFunction}"
To="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformTranslateY}, TargetNullValue=0, FallbackValue=0}" />
<DoubleAnimation Storyboard.TargetName="HeaderElement_CompositeTransform"
Storyboard.TargetProperty="ScaleX"
Duration="{StaticResource MaterialTextBoxAnimationDuration}"
EasingFunction="{StaticResource MaterialEaseInOutFunction}"
To="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformScales}, TargetNullValue=1, FallbackValue=1}" />
<DoubleAnimation Storyboard.TargetName="HeaderElement_CompositeTransform"
Storyboard.TargetProperty="ScaleY"
Duration="{StaticResource MaterialTextBoxAnimationDuration}"
EasingFunction="{StaticResource MaterialEaseInOutFunction}"
To="{Binding RelativeSource={RelativeSource TemplatedParent}, Converter={StaticResource HeaderCompositeTransformScales}, TargetNullValue=1, FallbackValue=1}" />
</Storyboard>-->

cc @kazo0 @Xiaoy312

@eriklimakc eriklimakc force-pushed the dev/ERLI/1451-textbox-disabled branch from 7c96d93 to 3639ddb Compare August 7, 2024 10:41
@agneszitte agneszitte requested a review from Xiaoy312 August 7, 2024 17:50
@eriklimakc eriklimakc marked this pull request as ready for review August 8, 2024 13:13
Copy link
Contributor

@Xiaoy312 Xiaoy312 left a comment

Choose a reason for hiding this comment

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

while there is the concern of having multiple parallel visual states (from different VSG) competing to modify the same dependency property
but going through the msdn docs on VS,VSG,VSM,Storyboard,Timeline,DoubleAnimation,Storyboarded animations, etc...
microsoft doesnt seem to explicitly forbid this practice
so ill greenlight this, since it seems to work well atm

@Xiaoy312
Copy link
Contributor

Xiaoy312 commented Aug 8, 2024

@eriklimakc would you be able to create a standalone repro of the wasm textbox problem, or save a snapshot branch in themes
and open an issue on uno, so we could investigate further?

@eriklimakc
Copy link
Contributor Author

@eriklimakc would you be able to create a standalone repro of the wasm textbox problem, or save a snapshot branch in themes and open an issue on uno, so we could investigate further?

@Xiaoy312 Yes, here is the issue unoplatform/uno#17888

cc @kazo0

@eriklimakc eriklimakc changed the title fix: Place header placeholder to proper place when disabled fix: ensure attribute animations respond to code changes Aug 8, 2024
@kazo0 kazo0 merged commit 4fa6108 into master Aug 9, 2024
20 checks passed
@kazo0 kazo0 deleted the dev/ERLI/1451-textbox-disabled branch August 9, 2024 19:06
mergify bot pushed a commit that referenced this pull request Aug 9, 2024
Xiaoy312 added a commit that referenced this pull request Aug 12, 2024
….1/pr-1452

fix: ensure attribute animations respond to code changes (backport #1452)
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.

[Material] TextBox Header and Text overlapped with IsEnabled set to false
4 participants