Skip to content

ZEPPELIN-2486. AngularElem's onChange is only invoked once - #2312

Closed
zjffdu wants to merge 1 commit into
apache:masterfrom
zjffdu:ZEPPELIN-2486
Closed

ZEPPELIN-2486. AngularElem's onChange is only invoked once#2312
zjffdu wants to merge 1 commit into
apache:masterfrom
zjffdu:ZEPPELIN-2486

Conversation

@zjffdu

@zjffduzjffdu commented May 3, 2017

Copy link
Copy Markdown
Contributor

What is this PR for?

ng-change can not capture event, so that means $event.timeStamp is undefined. angular/angular.js#6370
This cause AngularElem's onChange is only invoked once. This PR use another approach to update the angularObject.

What type of PR is it?

[Bug Fix]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Tested manually

Screenshots (if appropriate)

Before
zeppelin_before
After
zeppelin_after

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@zjffdu

Copy link
Copy Markdown
ContributorAuthor

@Leemoonsoo Please help review.

Text(s"${functionName}=$$event.timeStamp"),
Null)
Text(s"${functionName}=${functionName} + 1"),
Null) % Attribute(None, "ng-init", Text(s"${functionName} = 0"), Null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like initializing value using ng-init calls callback function once, before the actual event.
I think the ${functionName} need to be initialized in the following line

val angularObject = addAngularObject(functionName, 1)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good catch, PR is updated.

@Leemoonsoo

Copy link
Copy Markdown
Member

LGTM

@zjffdu

Copy link
Copy Markdown
ContributorAuthor

Thanks, will merge if no more comments.

@asfgitasfgit closed this in 92e3abcMay 4, 2017
Sign up for freeto 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.

2 participants

@zjffdu@Leemoonsoo