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.

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

Separated some UI components into @optuna/react #851

Merged
merged 41 commits into from
Apr 3, 2024

Conversation

porink0424
Copy link
Collaborator

@porink0424 porink0424 commented Mar 29, 2024

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.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

NA

What does this implement/fix? Explain your changes.

  • Separated some UI components into @optuna/react as the first step to make codes common
    • In this PR, the following components were moved into @optuna/react as common codes:
      • DataGrid
      • PlotHistory
      • TrialTable
      • PlotlyDarkMode

@porink0424 porink0424 requested a review from c-bata March 29, 2024 02:45
@porink0424 porink0424 changed the title Separated some UI components into @optuna/storybook Separated some UI components into @optuna/react Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.51%. Comparing base (978695b) to head (15afdb9).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #851   +/-   ##
=======================================
  Coverage   69.51%   69.51%           
=======================================
  Files          35       35           
  Lines        2375     2375           
=======================================
  Hits         1651     1651           
  Misses        724      724           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@c-bata c-bata self-assigned this Mar 29, 2024
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

Changes looks almost good to me. I left some suggestions though.

biome.json Outdated
@@ -14,6 +14,7 @@
],
"ignore": [
"optuna_dashboard/ts/components/PlotlyColorTemplates.ts",
"tslib/react/src/components/PlotlyDarkMode.ts",
"standalone_app/src/PlotlyDarkMode.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Can we just remove this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed L18 ("standalone_app/src/PlotlyDarkMode.ts"), not L17 ("tslib/react/src/components/PlotlyDarkMode.ts"), thank you for your catch!

Comment on lines 2 to 37
"name": "optuna-dashboard-wasm",
"private": true,
"version": "0.0.0",
"description": "",
"scripts": {
"watch": "vite",
"build:vscode": "webpack"
},
"devDependencies": {
"@types/plotly.js": "^2.29.2",
"@types/react": "^18.2.64",
"@types/react-dom": "^18.2.21",
"@types/react-router-dom": "^5.3.3",
"@vitejs/plugin-react": "^4.2.1",
"compression-webpack-plugin": "^11.1.0",
"esbuild-loader": "^4.0.3",
"vite": "^5.1.5",
"webpack": "^5.90.3",
"webpack-cli": "^5.1.4"
},
"dependencies": {
"@emotion/react": "^11.11.3",
"@emotion/styled": "^11.11.0",
"@mui/icons-material": "^5.15.12",
"@mui/lab": "^5.0.0-alpha.167",
"@mui/material": "^5.15.12",
"@optuna/react": "../tslib/react",
"@optuna/storage": "../tslib/storage",
"module-workers-polyfill": "^0.3.2",
"notistack": "^3.0.1",
"optuna": "../rustlib/pkg",
"plotly.js-dist-min": "^2.30.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-router-dom": "^6.22.3"
}
Copy link
Member

Choose a reason for hiding this comment

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

[question] Should we use a tab for the indentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No specific reason for my change here, so I replaced tab with spaces.
Thanks for your comment.

@@ -0,0 +1,45 @@
import { JournalFileStorage, SQLite3Storage } from "@optuna/storage"
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this file into tslib/storage or just leaving it in the standalone_app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will leave this here as it is, because loadStorageFromFile function is used in MockStudies,
but certainly this file should not be included in the package.
So I marked this file into exclude in tsconfig.json.

Comment on lines 38 to 40
"optionalDependencies": {
"@rollup/rollup-linux-x64-gnu": "^4.13.2"
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Memo:These lines are added to resolve the error due to rollup dependency (module not found error: Error: Cannot find module @rollup/rollup-linux-x64-gnu)

ref: https://stackoverflow.com/questions/77569907/error-in-react-vite-project-due-to-rollup-dependency-module-not-found

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM!

@c-bata c-bata merged commit 507a7c0 into optuna:main Apr 3, 2024
11 checks passed
c-bata added a commit that referenced this pull request Apr 12, 2024
Separated UI components into `@optuna/react` (Continued from #851)
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.

2 participants