[Bug] Ratings Prompt: Animations #76
Conversation
…pper into feature/ratings-prompt-animations
…ent to animate (for better reusability)
…ttons work; fix animation perf issue
| @@ -1,3 +1,5 @@ | |||
| import {SmartValue} from "../../communicator/smartValue"; | |||
There was a problem hiding this comment.
I think I am abusing the intended usage of SmartValues in this PR... I started using SVs because at one point during development, I thought I needed to be able to subscribe to changing AnimationStates.
Then it turned out I didn't need to subscribe, but I still needed a value container to 1) persist ratings panel AnimationState whenever the ratings panel (and its state) is blown away randomly by a re-render; and 2) update this persisted value across different layers of the animation strategy code (via setAnimationStrategy).
If someone has a suggestion for getting these behaviors without using SVs (perhaps there's a more Mith-react way?), please let me know :) Thanks
There was a problem hiding this comment.
I'm about to go through this PR so I will report back. My initial thought is to move state up one component so that it persists across re-renders of a sub-component, and pass it as a prop down the different layers of the animation strategy code.
There was a problem hiding this comment.
Now that I think about, a Mith-react component tree serves a similar purpose to a SmartValue. When the state changes, it re-renders the component and passes the re-rendered value down as a prop to its children. #FoodForThought
There was a problem hiding this comment.
Good thought that I also had :D
So I originally tried implementing this by putting AnimationState (in enum form) on the MainController state. But I don't think that's actually what we want here because we don't want the main controller re-rendering on every animation state update of the sub-panel - we only want RatingsPanel to re-render.
Also note that I already am storing this on MainController state and then passing it to RatingsPanel as a prop, as suggested... but I'm avoiding the re-render behavior because I update the internals of the smart value and not the SmartValue<AnimationState> itself :P So that's another reason SVs are (too?) convenient here.
…anel animation in main controller with a simple fade-in
|
Build is broken when I merge master into this branch and run |
|
^ @VishaalK and I were chatting about this and this is because the way we did typings, and what was in package.json is different in master compared to what is in this branch. You'll need to merge master into this branch and resolve typings, packages, and failing UTs :) |
|
^what happens when a PR sits around for too long I guess :) |
|
The animations look really good :D |
|
|
||
| protected doAnimateIn(parentEl: HTMLElement): Promise<void> { | ||
| return new Promise<void>((resolve) => { | ||
| for (let cIndex = 0; cIndex < this.contentToAnimate.length; cIndex++) { |
There was a problem hiding this comment.
Personally, I prefer to call these variables contentIndex and animatableIndex
There was a problem hiding this comment.
can do - agreed that this is a little confusing #Resolved
| Rate, | ||
| Feedback, | ||
| End | ||
| Init = 0, |
There was a problem hiding this comment.
Is there a reason why you explicitly define the index for each enum value?
There was a problem hiding this comment.
just to further call out that order matters for this particular enum
|
Created #93 to track this fix |
Adds the new motion designer-approved animations for the ratings prompt feature.
Some restructuring of the existing animations framework was needed.