-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add TableArtifactViewer for csv file #877
Conversation
ba36454
to
2114f96
Compare
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.
Thank you very much for the PR 👍
I left some comments.
|
||
useEffect(() => { | ||
const handleFileChange = async () => { | ||
const loadedData = await loadCSV(props) |
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.
You may want to catch errors occurred in loadCSV
. I think it would be better to display the error message.
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.
Added error handling.
@@ -24,6 +24,7 @@ import { StudyDetail } from "ts/types/optuna" | |||
import { actionCreator } from "../../action" | |||
import { ArtifactCardMedia } from "./ArtifactCardMedia" |
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.
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.
Stop showing table here. Just leave file open button.
<Modal | ||
open={open} | ||
onClose={() => { | ||
setOpen(false) | ||
setTarget(["", null]) | ||
}} | ||
> | ||
<Box | ||
component="div" | ||
sx={{ | ||
position: "absolute", | ||
top: "50%", | ||
left: "50%", | ||
transform: "translate(-50%, -50%)", | ||
bgcolor: "background.paper", | ||
borderRadius: "15px", | ||
}} | ||
> | ||
<IconButton | ||
sx={{ | ||
position: "absolute", | ||
top: theme.spacing(2), | ||
right: theme.spacing(2), | ||
}} | ||
onClick={() => { | ||
setOpen(false) | ||
setTarget(["", null]) | ||
}} | ||
> | ||
<ClearIcon /> | ||
</IconButton> | ||
<TableArtifactViewer | ||
src={target[0]} | ||
filetype={target[1]?.filename.split(".").pop()} | ||
/> | ||
</Box> | ||
</Modal> |
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.
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.
Make it scrollable.
@keisuke-umezawa |
@porink0424 |
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.
LGTM!
Contributor License Agreement
This repository (
optuna-dashboard
) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
NA
What does this implement/fix? Explain your changes.
Make artifact viewer compatible with CSV files.
sample
You can see artifact loader on study page.