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

[node-red-node-ui-vega] Append Data into a Vega Chart #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yasithdev
Copy link

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

In node-red charts, when new data is received, it automatically appends that to the chart.
However, in ui_vega, it requires sending a new vega spec (with the updated data) to achieve this.
This introduces a severe lag for a complex spec.

In this PR I added a new function to push just the data into a ui_vega node.
This enables adding new data without triggering a full redraw.

How I've done it (at this point) is by sending messages into the node in two forms: templates and data.
Initially, I wanted to differentiate between them using msg.topic, but for some reason, it showed up as undefined.
For now, I've hacked around it by checking if msg.payload.$schema is present in the payload.

Any design revisions would be appreciated!

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@dceejay dceejay requested a review from HiroyasuNishiyama May 25, 2021 08:03
@yasithdev
Copy link
Author

Hello all, any update on this? Please let me know if you suggest any modifications.

@dceejay
Copy link
Member

dceejay commented Jul 2, 2021

@HiroyasuNishiyama - this looks ok to me - but you guys are the main creators and users of this. OK with you ?

@@ -84,7 +94,11 @@ loadScripts(["https://cdn.jsdelivr.net/npm/vega@5.20.2",
if (!vegaEmbed) {
return;
}
showVega(msg.payload);
if (msg.payload && msg.payload.$schema) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this fix break the compatibility of existing code because $schema property is not mandatory?
May I suggest adding msg.command property for setting data structure for controlling Vega specification?

Copy link
Member

Choose a reason for hiding this comment

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

I'm very sorry.  
I didn't realize that I had failed to send the review results.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea. I'll test it out with a msg.command property and update the PR.

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.

3 participants