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.

Sign up for GitHub

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

fix(finalize): call embed finalize instead of view #421

Merged
merged 6 commits into from
Aug 27, 2021
Merged

fix(finalize): call embed finalize instead of view #421

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2c4c16
fix(finalize): call embed finalize instead of view
pelikhan Aug 6, 2021
70e2ffe
fix: property handle Result | undefined
pelikhan Aug 6, 2021
1787e08
fix: styling
pelikhan Aug 6, 2021
b1400c9
fix: handle missing result
pelikhan Aug 6, 2021
0cff790
fix: remove spurious true
pelikhan Aug 6, 2021
841d7b6
fix: avoid "?."
pelikhan Aug 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 26 additions & 16 deletions packages/react-vega/src/VegaEmbed.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { CSSProperties } from 'react';
import vegaEmbed, { EmbedOptions, VisualizationSpec } from 'vega-embed';
import { ViewListener, View, SignalListeners } from './types';
import type { Result } from 'vega-embed';
import { ViewListener, SignalListeners } from './types';
import shallowEqual from './utils/shallowEqual';
import getUniqueFieldNames from './utils/getUniqueFieldNames';
import { NOOP } from './constants';
Expand All @@ -21,7 +22,7 @@ export type VegaEmbedProps = {
export default class VegaEmbed extends React.PureComponent<VegaEmbedProps> {
containerRef = React.createRef<HTMLDivElement>();

viewPromise?: Promise<View | undefined>;
resultPromise?: Promise<Result | undefined>;

componentDidMount() {
this.createView();
Expand Down Expand Up @@ -102,11 +103,11 @@ export default class VegaEmbed extends React.PureComponent<VegaEmbedProps> {
};

modifyView = (action: ViewListener) => {
if (this.viewPromise) {
this.viewPromise
.then((view) => {
if (view) {
action(view);
if (this.resultPromise) {
this.resultPromise
Copy link
Member

Choose a reason for hiding this comment

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

At some point, we should rewrite this to use a sync/await but that's out of scope for this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried to keep the diff to a minimum set of changes

.then((result) => {
if (result) {
action(result.view);
}

return true;
Expand All @@ -119,12 +120,15 @@ export default class VegaEmbed extends React.PureComponent<VegaEmbedProps> {
const { spec, onNewView, signalListeners = {}, width, height, ...options } = this.props;
if (this.containerRef.current) {
const finalSpec = combineSpecWithDimension(this.props);
this.viewPromise = vegaEmbed(this.containerRef.current, finalSpec, options)
.then(({ view }) => {
if (addSignalListenersToView(view, signalListeners)) {
view.run();
this.resultPromise = vegaEmbed(this.containerRef.current, finalSpec, options)
.then((result) => {
if (result) {
const { view } = result;
if (addSignalListenersToView(view, signalListeners)) {
view.run();
}
}
return view;
return result;
})
.catch(this.handleError);

Expand All @@ -135,10 +139,16 @@ export default class VegaEmbed extends React.PureComponent<VegaEmbedProps> {
}

clearView() {
this.modifyView((view) => {
view.finalize();
});
this.viewPromise = undefined;
if (this.resultPromise) {
this.resultPromise
.then((result) => {
if (result) {
result.finalize();
}
})
.catch(this.handleError);
}
this.resultPromise = undefined;

return this;
}
Expand Down