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: テーマ周りのコードを整理し、設定を取得する汎用関数に合流させたり、テーマ変更関数を切り出したりした #2284

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Oct 5, 2024

内容

かなり前に実装されていたテーマ周り(デフォルトテーマ・ダークテーマ)のコードを少し整理しました。
やったことは主に3つです。

  • 設定を取得する・保存する部分を汎用の関数に合流させ、汎用関数で足りない部分だけ実装するようにした
    • 以前はセッター兼ゲッターな特殊な関数を使っていた
    • 今は設定を保存する・取得する部分は既存コードがあるので、そっちを使うように変更
    • 利用可能なテーマを取得する関数だけは別で用意
  • DOMに色情報を登録するコードを切り出しました
    • と言っても関数に移動しただけ
    • テーマに依存したコンポーネントのstorybook用意をちょっとだけ容易にするため
      • 実際は結構がっつりモックを作らないといけないので、その便利ができるまではちょっと不便
  • VuexでDOMを変更するのではなく、Vuex.stateをwatchする.vue内で行うようにしました
    • UI部分は.vue側に集約しときたい気持ちがちょっとあったので
    • これは意味があったのかは賛否両論かも
    • コンポーネントに切り出せばstorybookで便利・・・かも・・・?
    • 微妙そうだったら戻します

その他

Comment on lines +91 to +92
main ブランチの Storybook は Chromatic から確認できます。
<https://main--667d9c007418420dbb5b0f75.chromatic.com/>
Copy link
Member Author

Choose a reason for hiding this comment

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

そういえばchromaticがあるのを紹介しなかったので、READMEに追記しておきました。

src/store/type.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Oct 12, 2024

セルフマージしようとしています!
チェックリストだけ作ろうかなと。

  • フォントが変更できる
  • フォントの変更が設定に保存されて、起動時からちゃんとフォントが変更できる
  • テーマが変更できる
  • テーマの変更が設定に保存できて、起動時からちゃんとテーマが変更されている
  • ブラウザ版でもテーマが変更できる
  • コードの移動の部分がちゃんとコピペになっていること確認

@Hiroshiba
Copy link
Member Author

問題ないと思うのでマージします!

@Hiroshiba Hiroshiba merged commit 4bb13ca into VOICEVOX:main Oct 12, 2024
8 checks passed
@Hiroshiba Hiroshiba deleted the テーマ周りのコードをリファクタリング branch October 12, 2024 13:34
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.

1 participant