Skip to content
This repository was archived by the owner on Jun 10, 2024. It is now read-only.

Git 150 initial draft Tex render#175

Open
habiblawal1 wants to merge 8 commits intoodpi:mainfrom
habiblawal1:git150
Open

Git 150 initial draft Tex render#175
habiblawal1 wants to merge 8 commits intoodpi:mainfrom
habiblawal1:git150

Conversation

@habiblawal1
Copy link
Copy Markdown

No description provided.

Signed-off-by: habib <habiblawal@hotmail.co.uk>
@habiblawal1 habiblawal1 marked this pull request as ready for review May 18, 2023 14:45
@habiblawal1 habiblawal1 requested a review from sarbull as a code owner May 18, 2023 14:45
Signed-off-by: habib <habiblawal@hotmail.co.uk>
Signed-off-by: habib <habiblawal@hotmail.co.uk>
Copy link
Copy Markdown
Member

@sarbull sarbull left a comment

Choose a reason for hiding this comment

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

this should not be merged as it is now :) because it will break the current functionality and only work for tex

<h1>TEx</h1>
</div>

<Modal
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.

extract tex functionality in separate demo component so that we can differentiate and encapsulate the functionality

ideally the modal should be implemented in the egeria-ui-components where there will be a new happi-graph instance

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've changed the frontend to have a dropdown menu to choose the graph type, the only issue I'm struggling with a bit is to have the Happi Graph update with the new Graph Type state after you change it with the dropdown. If you run the code it should make sense. For example, whatever graph type you start with is ok, but then if you change the graph via the down from Inheritance to Lineage, the graph does not refresh to show the Lineage graph.

@@ -0,0 +1,565 @@
import * as d3 from 'd3';
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.

duplicating the entire code is not ok :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes you're right, I've updated this in my recent commit

@@ -0,0 +1,565 @@
import * as d3 from 'd3';
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.

duplicating the entire code is not ok, please remove if not applicable

const mappedNodes = mapNodes(props.rawData.nodes, props.selectedNodeId);
const mappedLinks = mapLinks(props.rawData.edges, mappedNodes);

let selectedGraphType;
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.

extract this to separate function, make it more readable


addNodes(nodes, nodesGroup, graphDirection, onNodeClick);
addLinks(links, linksGroup, graphDirection, nodes);
this.state.addNodes(nodes, nodesGroup, graphDirection, onNodeClick);
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.

remove this.state

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants