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(ivy): setting up animation properties correctly (FW-643) #27496

Closed
wants to merge 6 commits into
base: master
from

Conversation

@AndrewKushnir
Contributor

AndrewKushnir commented Dec 6, 2018

Prior to this change, animation properties were defined as element attributes, which caused errors at runtime. Now all animation-related attributes are defined as element properties.

Also as a part of this update, we start to account for bindings used in animations, which was previously missing.

Note: the data.animations was changed to data.animation, since this name is expected in createRenderer function - that seems like a typo.

P.S.: resolving this issue revealed 3 other issues. I did preliminary investigation, created tickets and mentioned them in animation_renderer_spec.ts.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

AndrewKushnir added some commits Dec 5, 2018

fix(ivy): setting up animation properties correctly (FW-643)
Prior to this change, animation properties were defined as element attributes, which caused errors at runtime. Now all animation-related attributes are defined as element properties.

Also as a part of this update, we start to account for bindings used in animations, which was previously missing.

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-643_animations_set_property branch from 549cab3 to 2740e09 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@kara

kara approved these changes Dec 6, 2018 edited

LGTM (once other comments are addressed)

if (isAnimationProp(attrName)) {
if (isProc) {
(renderer as ProceduralRenderer3).setProperty(native, attrName, attrVal);
}

This comment has been minimized.

@JoostK

JoostK Dec 6, 2018

Contributor

If we don't have an else here, this won't throw for animation triggers when no animations module is present (as it may not be a procedural renderer then). This is a deviation from ViewEngine.

Would it be an idea to introduce an instruction for animations and not consider them as regular attributes? That offers more freedom in the future.

This comment has been minimized.

@matsko

matsko Dec 6, 2018

Member

Hey @JoostK. At this very moment we don't know exactly how the @animation attrs will be passed into the non-renderer case (when only document is active). We don't want to introduce anything temporary until the design is figured out therefore the single if statement case is the best we can do for now.

This comment has been minimized.

@JoostK

JoostK Dec 6, 2018

Contributor

@matsko That makes sense, thanks for the reply!

(renderer as ProceduralRenderer3)
.setAttribute(native, attrName as string, attrVal as string) :
native.setAttribute(attrName as string, attrVal as string);
}

This comment has been minimized.

@JoostK

JoostK Dec 6, 2018

Contributor

Consider rewriting this logic to become the same as elementProperty, we could reduce the two isProc checks and calls to a single one.

This comment has been minimized.

@AndrewKushnir

AndrewKushnir Dec 6, 2018

Contributor

Thanks for reviewing this PR. You are right, there are few places where this logic exists in instructions.ts, but I'd prefer to have another cleanup/refactor PR to separate these changes (I'll take a note on that). Thank you.

@AndrewKushnir

This comment has been minimized.

Contributor

AndrewKushnir commented Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@IgorMinar IgorMinar removed the request for review from matsko Dec 6, 2018

@IgorMinar IgorMinar closed this in c71d7b5 Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment