Skip to content

[ZEPPELIN-2234] Can't display the same chart again (master) - #2110

Closed
1ambda wants to merge 2 commits into
apache:masterfrom
1ambda:ZEPPELIN-2234/cant-display-same-chart-again
Closed

[ZEPPELIN-2234] Can't display the same chart again (master)#2110
1ambda wants to merge 2 commits into
apache:masterfrom
1ambda:ZEPPELIN-2234/cant-display-same-chart-again

Conversation

@1ambda

Copy link
Copy Markdown
Member

What is this PR for?

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

  • This should be backported into 0.7.0 as well.

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=newVisualization(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

@soralee

Copy link
Copy Markdown
Contributor

Let me test master and branch-0.7 out!

@Leemoonsoo

Copy link
Copy Markdown
Member

Tested and it fixes the problem.

However, since the change #2092, visualization is re-created everytime when switching. I'd suggest find a way restore ng-show and address problem ZEPPELIN-2148 in different way. So we can leverage activate() deactivate() function. Otherwise it is bit annoying especially with visualization that takes long time to create/initialize.

@1ambda

Copy link
Copy Markdown
MemberAuthor

@Leemoonsoo Thanks for review.

Then I will revert 1bdab41 and #2092 while trying to use ng-show instead of.

@1ambda1ambda changed the title [ZEPPELIN-2234][BUG] Can't display the same chart again (master)[ZEPPELIN-2234][WIP] Can't display the same chart again (master)Mar 10, 2017
@1ambda
1ambdaforce-pushed the ZEPPELIN-2234/cant-display-same-chart-again branch from 1bdab41 to 14ee617CompareMarch 10, 2017 06:33
@1ambda

Copy link
Copy Markdown
MemberAuthor

I'v just updated code. @Leemoonsoo Could you review again? Thanks!

@Leemoonsoo

Copy link
Copy Markdown
Member

Tested and LGTM!

@prabhjyotsingh

Copy link
Copy Markdown
Contributor

@1ambda Thank you for fixing this.
Tested on local, works well. LGTM.

@Leemoonsoo

Copy link
Copy Markdown
Member

Merge to master if no further discussions.

@1ambda

Copy link
Copy Markdown
MemberAuthor

Thanks @Leemoonsoo@prabhjyotsingh!

@1ambda1ambda changed the title [ZEPPELIN-2234][WIP] Can't display the same chart again (master)[ZEPPELIN-2234] Can't display the same chart again (master)Mar 10, 2017
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
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.

4 participants

@1ambda@soralee@Leemoonsoo@prabhjyotsingh