Skip to content

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

Closed
1ambda wants to merge 2 commits intoapache: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 intoapache:masterfrom
1ambda:ZEPPELIN-2234/cant-display-same-chart-again

Conversation

@1ambda
Copy link
Copy Markdown
Member

@1ambda 1ambda commented Mar 8, 2017

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

@soralee
Copy link
Copy Markdown
Contributor

soralee commented Mar 9, 2017

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
Member Author

1ambda commented Mar 10, 2017

@Leemoonsoo Thanks for review.

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

@1ambda 1ambda 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 1ambda force-pushed the ZEPPELIN-2234/cant-display-same-chart-again branch from 1bdab41 to 14ee617 Compare March 10, 2017 06:33
@1ambda
Copy link
Copy Markdown
Member Author

1ambda commented Mar 10, 2017

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
Member Author

1ambda commented Mar 10, 2017

Thanks @Leemoonsoo @prabhjyotsingh!

@1ambda 1ambda 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 asfgit closed this in ce97b53 Mar 11, 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 <[email protected]>

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 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.

4 participants