-
Notifications
You must be signed in to change notification settings - Fork 310
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
[project-library-manage] 音声ライブラリ管理画面のモックを追加 #1453
[project-library-manage] 音声ライブラリ管理画面のモックを追加 #1453
Conversation
Co-Authored-By: sevenc-nanashi <59691627+sevenc-nanashi@users.noreply.github.com>
e5470b9
to
962a3d9
Compare
おつです! ちょっと関連issueのとこに音声ライブラリのプロジェクトのリンクを貼っときました! 最近忙しいのとプルリクエストが多いのでちょっと見るのが遅れそうに感じています 🙇 @sevenc-nanashi さん |
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.
試聴できました!!!! 良いですねー!!!!!!
このプルリクエストのゴールとしてはどこにしましょうか?
デザインもしっかりしてからマージするか、一旦デザイン周りは後回しにペンディングしてタスクリストなどに追加しといて一旦マージするかなどの選択肢はありそうです!
とりあえず気になったところとしては @sevenc-nanashi さんがおっしゃっていたところと、あとキャラクターをクリックした時に左にキャラクター欄が出て来るので、最初から何もない空間、あるいは一番左上のキャラクターが表示されていた方がユーザーが驚かないかなと思いました!
if (!installedLibrary) { | ||
return false; | ||
} |
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.
ここってこの条件はありえないという感じですよね。だったらエラーを握りつぶしてしまっていることになるので、throwのが適していると思います!
参考になる記事
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.
ここの条件は握り潰しではなく、あり得る条件ですね。
「インストールされていないが、これからインストール予定のライブラリ」がlatestではないのは自明なので、この形は問題ないはずです。
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.
なるほどです! 良いと思います!
ただの気づきですが、状態(latest/installed/notIntalled)と可能なアクション(update/uninstall/install)があって、この2つを変換するだけの関数があると、その関数のテストが書けるんだなーと思いました。
今は状態からアクションの変換をtemplate内でやってるから、状態からレンダリングのテストを統合的に行うテストになるなーと。
Co-authored-by: Nanashi. <sevenc7c@sevenc7c.com>
デザイン、名無しさんが挙げられている形が一番すっきりしているように見えました...! せっかくなのでこの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!
Co-authored-by: Nanashi. <sevenc7c@sevenc7c.com>
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!!!!
UI面に関して、今は横スクロールでいろんなキャラクターが表示されるようになっていますが、折り返して全キャラが表示されるようになってもいいかもと思いました。
ただ実際はもっともっと数が並ぶはずで、その時にどうなるかわからないので、今は一旦この形がベストなのかなと感じました!
キャラクター表示欄のところ、誰も選択されていない状態で無を表示することを提案しましたが、実際見てみるとさすがに何も表示されてないのはよくないのかもしれないとちょっと思いました。。。
最終的なデザインの調整で @y-chan さんの最初のプルリクエストの感じに戻させていただくかもしれません。すいません。。 🙇
(一番左上のキャラクターの自動選択が可能そうだったらそっちにしたみです)
内容
題のとおりです。音声ライブラリ管理画面のモックを追加しました。
実際に音声ライブラリのインストールはできませんが、キャラ音声の試聴はできるようにしておきました。
関連 Issue
スクリーンショット・動画など
その他
動作確認に、音声ライブラリ管理機能を使えるようにしたSHAREVOXエンジンを使っています。
以下からダウンロードして使えます。
https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.1