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.

Sign up for GitHub

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

Jump to bottom

Graph view #76

Merged
merged 39 commits into from
May 12, 2021
Merged

Graph view #76

merged 39 commits into from
May 12, 2021

Conversation

CatoLeanTruetschel
Copy link
Collaborator

@CatoLeanTruetschel CatoLeanTruetschel commented May 9, 2021 •
edited
Loading

This PR adds a rundimentary graph view as described by #75 in order to implement #28.
Closes #75
Closes #28

@CatoLeanTruetschel CatoLeanTruetschel added this to the Sprint 4 milestone May 9, 2021
@CatoLeanTruetschel CatoLeanTruetschel self-assigned this May 9, 2021
@CatoLeanTruetschel
Copy link
Collaborator Author

CatoLeanTruetschel commented May 11, 2021 •
edited
Loading

This now works but there are still some things to do

  • Consistently get the base path of the backend, without hardcoding it. This would be significantly simplified with Serve the frontend via the backend #60 - Done
  • Move the entities and helper function that are used in the frontend and in the backend to the shared folder - Done
  • Move the entity post processing from the frontend to the backend - Done
  • The resize of the window does not trigger a graph redraw reliably
  • Documentation of APIs - Done

@CatoLeanTruetschel
Copy link
Collaborator Author

This is not yet ready for merge, as there are some issues left. Maybe you can review already anyway @joluj and @PianoRollRepresentation.

joluj
joluj requested changes May 11, 2021
View reviewed changes
Copy link
Collaborator

@joluj joluj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Without any comments it's really hard to see whats going on.. The code looks good though.
The graph is shown, with the flaws you mentioned.

When the Code is documented then it's ready for the merge.

frontend/src/@types/react-graph-vis.d.ts Outdated Show resolved Hide resolved
frontend/src/entities/Edge.ts Outdated Show resolved Hide resolved
frontend/src/entities/EdgeDescriptor.ts Outdated Show resolved Hide resolved
frontend/src/routing/ListItemLink.tsx Outdated
import ListItemIcon from '@material-ui/core/ListItemIcon';
import ListItemText from '@material-ui/core/ListItemText';

interface ListItemLinkProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file was not created, nor changed by this PR

frontend/src/services/QueryService.ts Show resolved Hide resolved
frontend/src/services/QueryServiceImpl.ts
): Promise<QueryResult> {
const url = `${this.baseUri}/queryAll`;

return new Promise<QueryResult>((resolve, reject) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put all that "http-stuff" with cancel events etc. in a new class? We surely need this functionality in other methods as well.
But its good enough ATM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had this in mind, but as this is just needed in this single place currently did not invest the effort to actually factor it out yet.

@CatoLeanTruetschel CatoLeanTruetschel mentioned this pull request May 11, 2021
Closed
PianoRollRepresentation
PianoRollRepresentation reviewed May 11, 2021
View reviewed changes
Copy link
Collaborator

@PianoRollRepresentation PianoRollRepresentation left a comment

Choose a reason for hiding this comment

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

Same for me. Without comments it's difficult to understand...

@CatoLeanTruetschel
Copy link
Collaborator Author

All TODOs are resolved by now except for

The resize of the window does not trigger a graph redraw reliably

@CatoLeanTruetschel CatoLeanTruetschel marked this pull request as ready for review May 12, 2021 07:19
@CatoLeanTruetschel CatoLeanTruetschel merged commit 9d60825 into main May 12, 2021
@CatoLeanTruetschel CatoLeanTruetschel deleted the graph branch May 12, 2021 08:03
@CatoLeanTruetschel CatoLeanTruetschel mentioned this pull request May 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PianoRollRepresentation PianoRollRepresentation PianoRollRepresentation approved these changes

@joluj joluj joluj approved these changes

Assignees

@CatoLeanTruetschel CatoLeanTruetschel

Labels
None yet
Projects
None yet
Milestone
Sprint 4
Development

Successfully merging this pull request may close these issues.

Simple graph frontend Display context graph
3 participants
@CatoLeanTruetschel @joluj @PianoRollRepresentation