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

Refactor: SequencerGridをPresentation/Containerに分離 #2311

Conversation

sevenc-nanashi
Copy link
Member

内容

タイトル通りです。

関連 Issue

スクリーンショット・動画など

image

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner October 22, 2024 12:05
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team October 22, 2024 12:05
@Hiroshiba
Copy link
Member

テスト回すために空commitさせていただきます!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

完璧な切り出し差分だと思います、ありがとうございます!!!

@Hiroshiba Hiroshiba merged commit 428413c into VOICEVOX:main Oct 22, 2024
8 checks passed
@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 22, 2024

@romot-co @sigprogramming
たぶんPresentation/ContainerパターンはSing系で初めてだと思うので、ちょっと目的や利点を含めて説明させていただこうと思います!


目的は 見た目のテストをしやすくすること です。

VOICEVOXは大体のコンポーネントがVuexに依存していて、そのVuexを初期化するには結構な苦労が必要となっています。
そこで、1つのコンポーネントを、 外部のデータ=Vuexに依存するもの(Container) と、 見た目とUIの挙動だけを司るもの(Presentation) に分離するのがこのパターンです。
(コンテナ・プレゼンテーションパターンはこちらのブログがわかりやすいです。)

このパターンを使うとStorybookととても相性が良くなります。
今回のPRでできたStoryはStorybookで確認できて(npm run storybookでも見れます)、electronの起動をせずとも見た目を確認できます。
Controlsに書いてるパラメータを書き換えることで見た目が変わったり、上部のdark/lightを押してテーマを変えたり,
コンポーネントだけ先に作れたりと、いろいろ便利です。

あとクリック時の挙動のテストも書けたり、最近だと @sevenc-nanashi さんがスクショテストもできるようになり、リファクタリング前後で見た目が変わってないことを自動で確認してくれます。
今回は1パターンのスクショのみのテストですが、例えばもっとズームを引いたときの見た目とか、あとは変わった拍子にしたときの見た目とかもチェックできるはずです。

ただ最初はこのPRのようにファイル分離が必要で、コンフリクトは起きやすいはずです。
ということで、ちゃんと利点もあることをご紹介してみた次第でした・・・!! 🙏

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