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

fix: switched to use embed result finalize() #403

Closed
wants to merge 3 commits into from

Conversation

ndobb
Copy link

@ndobb ndobb commented May 19, 2021

Here's an attempt at a fix for #383. Please let me know if I've approached it wrong, etc.

Note I believe the contents of package.json are identical to master but git thinks there is a formatting change somewhere, apparently. In any case it should be unchanged to my knowledge.

@netlify
Copy link

netlify bot commented May 19, 2021

Deploy Preview for react-vega ready!

Built with commit 889934e

https://deploy-preview-403--react-vega.netlify.app

@ndobb ndobb mentioned this pull request May 19, 2021
@domoritz
Copy link
Member

This looks good. I think the code overall could be refactored, though. Maybe you can take a stab at it (either here or in a follow up). My idea is to use await and store the result instead of view and finalize separately. We did this refactoring in Svelte and I think it's a bit clearer. Check out: https://github.com/vega/svelte-vega/blob/59833d58a2f63511dc0a0498b049b5714c426044/packages/svelte-vega/src/VegaEmbed.svelte#L103

@ndobb
Copy link
Author

ndobb commented May 19, 2021

Sounds good and thanks for pointing that out. I've updated to follow the Svelte conventions https://github.com/uwrit/react-vega/blob/fix-finalize/packages/react-vega/src/VegaEmbed.tsx#L123.

@domoritz
Copy link
Member

Awesome. Thank you for the updates. @kristw can you take a look?

@domoritz domoritz requested a review from kristw May 19, 2021 17:30
@kristw
Copy link
Member

kristw commented Aug 27, 2021

replaced by #421

@kristw kristw closed this Aug 27, 2021
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.

None yet

3 participants