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

[ZEPPELIN-2234][BUG] Can't display the same chart again (branch-0.7) #2114

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Mar 9, 2017

What is this PR for?

Can't display the same chart again. I attached a screenshot.

and

Implementation Details

After #2092,

  • result.html will draw chart every time since we use ng-if instead of ng-show
  • that means DOM is deleted, and created too
  • so we have to create visualization instance every time which requires a newly created DOM.
builtInViz.instance = new Visualization(loadedElem, config); // `loadedElem` is the newly created DOM.

What type of PR is it?

[Bug Fix]

Todos

NONE

What is the Jira issue?

How should this be tested?

I attached a screenshot

Screenshots (if appropriate)

Before: buggy

2234

After: fixed

2234-2

Questions:

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

@Leemoonsoo
Copy link
Member

#2110 is merged to master. @1ambda is this PR ready for branch-0.7?

@1ambda
Copy link
Member Author

1ambda commented Mar 11, 2017

@Leemoonsoo Thanks for caring. The implementation we talked in #2110 is not applied yet. I will modify and comment soon.

@1ambda 1ambda force-pushed the ZEPPELIN-2234/cant-display-same-chart-again-for-070 branch from 140e79b to 936123d Compare March 15, 2017 20:37
@1ambda
Copy link
Member Author

1ambda commented Mar 15, 2017

Rebased and fixed using the implementation discussed in #2110

  • i didn't bring refactoring commits because 0.7.0-branch is not actively developed now.

@Leemoonsoo
Copy link
Member

@1ambda Thanks! LGTM and merge to branch-0.7 if no further discussions.

asfgit pushed a commit that referenced this pull request Mar 16, 2017
### What is this PR for?

Can't display the same chart again. I attached a screenshot.

- this is the same fix with #2110
- except refactoring PR
- based on branch-0.7

and

- CI failure might be related with #2103

#### Implementation Details

After #2092,

- result.html will draw chart every time since we use `ng-if` instead of `ng-show`
- that means DOM is deleted, and created too
- so we have to create visualization instance every time which requires a newly created DOM.

```js
builtInViz.instance = new Visualization(loadedElem, config); // `loadedElem` is the newly created DOM.
```

### What type of PR is it?
[Bug Fix]

### Todos

NONE

### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
* Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533]

### How should this be tested?

I attached a screenshot

### Screenshots (if appropriate)

##### Before: buggy

![2234](https://cloud.githubusercontent.com/assets/4968473/23694278/4451594e-041c-11e7-9971-f0bb5945a1be.gif)

##### After: fixed

![2234-2](https://cloud.githubusercontent.com/assets/4968473/23694270/34866ba8-041c-11e7-83a8-693a93646fa4.gif)

### Questions:
* Does the licenses files need update? - NO
* Is there breaking changes for older versions? - NO
* Does this needs documentation? - NO

Author: 1ambda <1amb4a@gmail.com>

Closes #2114 from 1ambda/ZEPPELIN-2234/cant-display-same-chart-again-for-070 and squashes the following commits:

936123d [1ambda] fix: Retry until graph DOM is ready
475b532 [1ambda] fix: Reert #2092 for 0.7.0
@Leemoonsoo
Copy link
Member

Merged to branch-0.7 but @asfgit does not automatically close this PR. @1ambda please close this PR manually.

@1ambda
Copy link
Member Author

1ambda commented Mar 17, 2017

@Leemoonsoo Thanks!

@1ambda 1ambda closed this Mar 17, 2017
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.

2 participants