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

feat(ivy): patch animations into metadata #25828

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@matsko
Member

matsko commented Sep 5, 2018

No description provided.

@googlebot googlebot added the cla: yes label Sep 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 5, 2018

@@ -1875,6 +1910,7 @@ class MockRendererFactory implements RendererFactory3 {
lastCapturedType: RendererType2|null = null;
createRenderer(hostElement: RElement|null, rendererType: RendererType2|null): Renderer3 {
debugger;

This comment has been minimized.

@SebastianPodgajny

SebastianPodgajny Sep 5, 2018

#25532
looks like this one is left behind 😃

This comment has been minimized.

@matsko

matsko Sep 6, 2018

Member

thank you.

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@matsko

This comment has been minimized.

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@@ -176,7 +184,7 @@ export class ComponentDecoratorHandler implements DecoratorHandler<R3ComponentMe
// analyzed and the full compilation scope for the component can be realized.
pipes: EMPTY_MAP,
directives: EMPTY_MAP,
wrapDirectivesInClosure: false,
wrapDirectivesInClosure: false, animations

This comment has been minimized.

@mhevery

mhevery Sep 6, 2018

Member

can you add a trailing , so that it wraps correctly.

}
};
const template = `animations: [{name: "foo123"}, {name: "trigger123"}]`;

This comment has been minimized.

@mhevery

mhevery Sep 6, 2018

Member

could you add restriction that this has to be in defineComponent

export class MyComponent {
}
@NgModule({declarations: [MyComponent]})

This comment was marked as resolved.

@mhevery

mhevery Sep 6, 2018

Member

don't collapse empty arrays.

@@ -246,6 +246,12 @@ export function compileComponentFromMetadata(
definitionMap.set('styles', o.literalArr(strings));
}
// e.g. `animations: [trigger('123', [])]`
if (meta.animations && meta.animations.length) {

This comment has been minimized.

@mhevery

mhevery Sep 6, 2018

Member

remove the empty array check

const animations: any[]|null = componentDefinition.animations || null;
let data = componentDefinition.data || {};
if (animations) {
data['animations'] = animations;

This comment has been minimized.

@mhevery

mhevery Sep 6, 2018

Member

I don't think this should be quoted unless the read is quoted as well.

type: AnimComp,
consts: 0,
vars: 0,
animations: [],

This comment has been minimized.

@mhevery

mhevery Sep 6, 2018

Member

if this is needed than move it to Rendere2 not to the plumbing which gets data to Renderer2

@mhevery

mhevery approved these changes Sep 6, 2018

Please resolve the issues before submitting.

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mhevery

mhevery approved these changes Sep 6, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Sep 6, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required status "code-review/pullapprove"
    pending status "google3" is pending
    pending status "ci/circleci: test_ivy_jit" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending status "ci/circleci: test" is pending
    pending status "continuous-integration/travis-ci/pr" is pending
    pending status "ci/circleci: integration_test" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar IgorMinar closed this in d2dfd48 Sep 7, 2018

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