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

[gsoc22] Fix realtime data update example #141

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

totallynotvaishnav
Copy link
Member

closes #140

@totallynotvaishnav
Copy link
Member Author

Currently, the hosted example doesn't make much sense as we have to set up the server locally. It would be nice if could host the server too. WDYT @nemesisdesign ?

@nemesifier
Copy link
Member

Currently, the hosted example doesn't make much sense as we have to set up the server locally. It would be nice if could host the server too. WDYT @nemesisdesign ?

I prefer to keep it simple and add instructions on how to test this locally and find a way to avoid showing it in the examples we host on github pages, is this doable?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Can you make sure the instructions in the README are up to date and work?
https://github.com/openwisp/netjsongraph.js/tree/gsoc22#realtime-update

@totallynotvaishnav
Copy link
Member Author

I prefer to keep it simple and add instructions on how to test this locally and find a way to avoid showing it in the examples we host on github pages, is this doable?

Yeah, we can return this example as a response when we start the server locally and remove it from gh-pages.

Can you make sure the instructions in the README are up to date and work?
https://github.com/openwisp/netjsongraph.js/tree/gsoc22#realtime-update

Will do.

@totallynotvaishnav totallynotvaishnav force-pushed the issues/140-fix-realtime-data-update branch from d283177 to a9dd861 Compare July 1, 2022 19:18
@totallynotvaishnav totallynotvaishnav added this to In progress in [GSoC 22] netjsongraph.js via automation Jul 1, 2022
@nemesifier
Copy link
Member

nemesifier commented Jul 4, 2022

Here it is how it was done in the old version:
https://github.com/openwisp/netjsongraph.js/blob/master/examples/custom-attributes.html
I was using a CSS class. I guess we can't do the same here.

We shouldn't hardcode a specific property for this, we have to look into a way to allow setting colors depending on a property which could be anything.

It's something like this doable?

const nodeColors = {
  default: 'black',
  gateway: 'orange'
}
const linkColors = {
  up: 'green',
  down: 'red'
}

new NetJSONGraph({
  prepareData: data => {
    data.nodes.map(node => {
        node.color = nodeColors[node.properties.gateway];
    });

    data.links.map(link => {
        link.color = linkColors[node.status];
    })
  }
});

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

If I open the examples locally, I see the real time link, if I click on it, I see Cannot GET /examples/netjson-updateData.html. Is this intended?

Can you disable the hover effect when hovering on nodes and links?

Can you get rid of that legend and have the nodes all of the same color?

Can you change the title to "Live update example"?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

```
npm install
Copy link
Member

Choose a reason for hiding this comment

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

for example here I would add:

cd examples/realtime_update

[GSoC 22] netjsongraph.js automation moved this from In progress to Review in progress Jul 4, 2022
@totallynotvaishnav
Copy link
Member Author

If I open the examples locally, I see the real time link, if I click on it, I see Cannot GET /examples/netjson-updateData.html. Is this intended?

Yeah, that example is not useful anymore. I forgot to remove the example from the index page.

Can you disable the hover effect when hovering on nodes and links?

Can you get rid of that legend and have the nodes all of the same color?

I thought of handling this in #113

@totallynotvaishnav
Copy link
Member Author

We shouldn't hardcode a specific property for this, we have to look into a way to allow setting colors depending on a property which could be anything.

It's something like this doable?

Yeah makes sense. I will implement something similar.

@totallynotvaishnav totallynotvaishnav force-pushed the issues/140-fix-realtime-data-update branch from 9af8155 to 3bae229 Compare July 5, 2022 18:04
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@totallynotvaishnav you forgot this:

Can you change the title to "Live update example"?

@@ -5,7 +5,7 @@
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"start": "node index.js"
"dev": "node index.js"
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't change this here unless we also update the README, why are you doing this schange?

@totallynotvaishnav totallynotvaishnav force-pushed the issues/140-fix-realtime-data-update branch from af25e00 to 9e2f2be Compare July 7, 2022 17:11
[GSoC 22] netjsongraph.js automation moved this from Review in progress to Reviewer approved Jul 7, 2022
@nemesifier nemesifier merged commit 2d55af8 into gsoc22 Jul 7, 2022
[GSoC 22] netjsongraph.js automation moved this from Reviewer approved to Done Jul 7, 2022
@nemesifier nemesifier deleted the issues/140-fix-realtime-data-update branch July 7, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants