Skip to content

Remove classed func execution on empty data#136

Merged
bzurkowski merged 1 commit into
masterfrom
Empty_graph_err
Dec 5, 2020
Merged

Remove classed func execution on empty data#136
bzurkowski merged 1 commit into
masterfrom
Empty_graph_err

Conversation

@k-jano
Copy link
Copy Markdown
Member

@k-jano k-jano commented Dec 2, 2020

This PR resolves bug described in #134 and #135. classed() func should not be executed on empty data.

@k-jano k-jano requested review from bzurkowski and filwie December 2, 2020 12:47
Copy link
Copy Markdown
Member

@bzurkowski bzurkowski left a comment

Choose a reason for hiding this comment

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

@k-jano One question.

Comment thread src/components/Graph.js
nodeIcon: nodeIcon,
nodeGroup: nodeGroup,
showLabels: !nodeLabel.classed('hidden'),
showLabels: nodeLabel._groups[0].length > 0 ? !nodeLabel.classed('hidden') : null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is _groups? Does it always contain at least one element ([0])?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

_groups is a property of d3 object, which contains (in our case) only one list containing all labels to be generated in the graph. If the list is empty, we should not execute .classed() function on the object, which occures with the error. In my code I wanted to stick to this object and check if it contains any values to display.

We could also think about checking if nodes list is empty, but ... we don't have guarantee that both values are empty in the same time in any case (for example some library bugs in corner cases).

@bzurkowski
Copy link
Copy Markdown
Member

@k-jano I checked in my testbed and the issue is only partially gone. There is no error when only alert type is selected. However, no objects are present in the graph at all:

Screenshot 2020-12-03 at 00 16 28

Note that there are 13 alerts reported in the system.

@bzurkowski
Copy link
Copy Markdown
Member

bzurkowski commented Dec 4, 2020

I guess that is the expected behavior since alerts without a source are not displayed in the graph. @k-jano Please, confirm 😉

@k-jano
Copy link
Copy Markdown
Member Author

k-jano commented Dec 4, 2020

@bzurkowski Exactly, this is expected behaviour, because we describe alert namespace based on the namespace of a source.

However this is an issue to solve. I think that you are right, there should be possibility to display only alert objects in the graph. I suggest to deliver this solution in separate PR, because:

  • it is not the problem explicit pointed in corresponding issues
  • there is a need to changing the graph behaviour, e.g. specifying alerts namespace based on full payload, not only rendered one

@bzurkowski
Copy link
Copy Markdown
Member

Fully agree. I reported an issue to track the proposed enhancement (#139). Let's merge this PR!

@bzurkowski bzurkowski merged commit 6ff0c05 into master Dec 5, 2020
@bzurkowski bzurkowski deleted the Empty_graph_err branch April 28, 2021 21:52
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.

Node is null when namespace does not contain node kind Error on removing last object type from selector

2 participants