-
Notifications
You must be signed in to change notification settings - Fork 0
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
PKG visualization - frontend #98
base: main
Are you sure you want to change the base?
Conversation
…up/pkg-api into feature/59-Visualization-frontend
assert response.json["message"] == "PKG visualized successfully." | ||
assert response.json["img_path"] == "tests/data/pkg_visualizations/test.png" | ||
|
||
with open("tests/data/pkg_visualizations/test.png", "rb") as img: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is failing. The path to the image is for some reason changed from relative to absolute when testing. I can't figure out how to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error seems to come from send_file
. Note that when using a relative path send_file
only look in Flask working directory. Maybe you can try to join the relative path used during testing with the root path to give an absolute path to send_file
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits to look at and questions for clarification.
# TODO: Update pkg.visualize_graph() to return partial graph based | ||
# on the query result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing dot at the end.
Can you create a follow-up issue and link it here?
<Route path="service" element={<div>Service Management</div>} /> | ||
<Route path="population" element={<div>Population form</div>} /> | ||
<Route | ||
path="preferences" | ||
element={<div>Personal Preferences</div>} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove these, they can be added in a PR related to the implementatio of their view.
<Nav.Item> | ||
<Nav.Link eventKey="/service" as={Link} to="/service"> | ||
Service Management | ||
</Nav.Link> | ||
</Nav.Item> | ||
<Nav.Item> | ||
<Nav.Link eventKey="/population" as={Link} to="/population"> | ||
Populate PKG | ||
</Nav.Link> | ||
</Nav.Item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, we may want to include these routes in related PRs.
import { useContext, useEffect, useState } from "react"; | ||
import axios from "axios"; | ||
import QueryForm from "./QueryForm"; | ||
import Button from "react-bootstrap/Button"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove unused import.
const PKGVisualization = () => { | ||
const { user } = useContext(UserContext); | ||
const [error, setError] = useState(""); | ||
const [image_path, setImagePath] = useState(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: usually in javascript, the variable name uses a capital letter instead of _
, e.g., imagePath
.
const { user } = useContext(UserContext); | ||
const [error, setError] = useState(""); | ||
const [image_path, setImagePath] = useState(""); | ||
const [query_info, setQueryInfo] = useState(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same comment as for image_path
.
}) | ||
.then((response) => { | ||
setError(""); | ||
console.log(response.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can be removed.
return ( | ||
<Container> | ||
<div> | ||
<b>Here you can execute your own SPARQL queries to manage your PKG.</b> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be queries to select statements within the PKG? From my understanding only SELECT queries are accepted.
<div> | ||
<b>This is your current PKG:</b> | ||
</div> | ||
{/* <div>[Only for testing] Local path to the image: {image_path}</div> */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can be removed.
@@ -2,7 +2,7 @@ import React, { useContext } from "react"; | |||
import "./App.css"; | |||
import APIHandler from "./components/APIHandler"; | |||
import LoginForm from "./components/LoginForm"; | |||
import Container from 'react-bootstrap/Container' | |||
import Container from 'react-bootstrap/Container'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use double quotes as other imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits to look at and questions for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits to look at and questions for clarification.
Closes #59