-
Notifications
You must be signed in to change notification settings - Fork 198
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
エンジン側でいろいろ設定できるGUIの追加 #531
Conversation
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.
少し気になった点をコメントしました。
また、READMEの方にこの機能について追記していただくと、ユーザーの方が使いやすいかと思います。
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.
横からですが、いくつか気になったところがあったのでコメントしました。
また、送信時にalertか何かでメッセージを出した方がわかりやすい気がします。
すみません。今年末で色々と立て込んでて、PRを出すのが遅くなりそうだったので、途中でしたが出した感じでした。週末でご指摘いただいた内容と続きをしていきますのでどうかお願いします。 |
PRありがとうございます!コードは簡潔で読みやすかったです!! |
確認を保存する前に行うようにしました。また、selectを使って選ぶ形式に変更しました。 |
Co-authored-by: takana-v <44311840+takana-v@users.noreply.github.com>
とりあえず、ご指摘いただいた内容に関して修正を行いました。 また、一つ質問なのですがBootstrapが通信で取得されるので、ローカルに保存してもいいのかなと思いましたがいかかでしょう。BootstrapはMITライセンスです。 |
7MBほどでしょうか。保存したほうが良さそうですね! もし慣れている、あるいは挑戦したい感じであればお任せしたいです! |
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.
実行してみました!!UIいい感じだと思います!
ちょっといくつか実装周りで気になったことが合ったのでコメントしてみました!
setting_loaderをif __name__ == "__main__"で作るように。
設定ファイルをユーザー辞書と同じように永続化できるようにし、 |
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
ご指摘された内容を修正し、README.mdにこれの記述を追加し、ビルドに対応しました。 |
READMEの方を修正しました。これで大丈夫だと思います。 テストの追加やローカルにbootstrapを保存するなどはまたPRを立てて行いたいと思います。 |
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!!
修正ありがとうございます、かなり良い感じになったのではと思いました!!
いくつかコメントしていますがあまり強い意見ではないです!
approveが付いたので、これ以降はレビュー内容の修正のみにして、処理や要素の追加は別PRにしていただけると助かります!
プレビューの内容を修正しました。二つは消し忘れでした。 |
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.
2点ほどコメントしました。
そこまで強い意見ではないので、変更が難しいようであればこのままでも大丈夫です。
今思ったのですが、設定ファイルの初期化機能あった方がいいですかね。 |
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!
初期化機能、良いと思うので、ぜひPRを送って頂ければと思います。
内容
設定GUIを追加を行いました
GUIの動作確認はChrome Firefox Edgeで行い、すべてにおいて正常に動作しました
関連 Issue
ref #522
スクリーンショット・動画など
WebUIのスクリーンショットです
その他