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

UI・.mdファイルの括弧の全角と半角を統一する #1510

Merged
merged 23 commits into from
Aug 28, 2023

Conversation

thiramisu
Copy link
Contributor

@thiramisu thiramisu commented Aug 18, 2023

内容

UIの括弧が全角と半角で分かれているのを統一します。
対象となるのは主にキャラクタースタイルの括弧です。

関連 Issue

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

その他

const formatCharacterStyleName = (characterName, styleName = DEFAULT_STYLE_NAME) => `${characterName} (${styleName})`; 的な関数を定義したかったんですが、逆にややこしくなりそうだったので諦めました…
表示的には(の前に半角スペースが入っているのが見やすいと思ったのですが、「テキストを繋げて書き出し」の出力結果に半角スペースが入ってなかったためです。
#1510 (review)
「テキストを繋げて書き出し」は、上記のコメントを参考に、書き出しは全角括弧に変更して、読み込みは全角・半角両方行えるようにしました。

@thiramisu thiramisu requested a review from a team as a code owner August 18, 2023 17:42
@thiramisu thiramisu requested review from Hiroshiba and removed request for a team August 18, 2023 17:42
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.

リファクタリングありがとうございます!!

全角括弧と半角括弧の統一は賛成です!!
ちょっと調べてみたのですが、全体的に全角括弧と半角括弧が混ざっていました。他にもキャラクター欄の上に表示してある場所は全角括弧になっていそうです。
個人的には日本語フォントなので日本語フォント用に設計された全角括弧の方がおそらくいいだろうなと思っているというのもあり、寄せるとしたら全角括弧が良いのかな・・・とか思ったのですがどうでしょうか・・・。

関数を定義したかったんですが、逆にややこしくなりそうだったので諦めました

こちらに関して、テキスト読み込みだけ半角括弧と全角括弧両対応ということにすれば、全部を全角括弧で統一できるかも・・・?


せっかくリファクタリングのプルリクエスト送っていただいているのに、コメントが多くて申し訳ないです。。

src/components/AudioInfo.vue Outdated Show resolved Hide resolved
src/components/AudioInfo.vue Outdated Show resolved Hide resolved
@thiramisu
Copy link
Contributor Author

thiramisu commented Aug 21, 2023

せっかくリファクタリングのプルリクエスト送っていただいているのに、コメントが多くて申し訳ないです。。

フロントエンドでUIが密接に関わってくる部分も多く、全然気にしてないどころか、自分だと方針決めかねていることも多いので、むしろどんどん言っていただけると助かります!

どっちかに寄せるのに賛成です。処理的には多分スペースを入れないほうがhtmlで自動省略されないので書きやすいのかなと思いました。
見た目的に半角/全角どちらが良いのかについてちょっとググってみたところだと…
政府の文書だと、括弧内の文字が半角/全角かによって半角/全角かが分かれてました。この仕様に則ると全角ですね。
コピーライターの方のnoteでも、エンジニアは半角をよく使うけど、コピーライター的には全角だと主張されていました(結論は統一すればどっちでも良いという感じですが)。
全角だと括弧の高さが日本語仕様なので収まりが良いことが触れられています。ただ、そのままだと横幅を取り、見た目的にも字間をツメた方が綺麗とも言われていました。
まあ全角で良いかなと思います。

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 21, 2023

@thiramisu 僕も気になって調べてみたのですが、全角統一で賛成です。全く同じ資料を読んでいました。

他にもデザイナーさんの資料を見た感じだと、半角カッコは重心が下にあるのでよくないと書かれてました。
https://yososhi.com/archives/how-designers-use-brackets/

あと念のためにUX・UIの方針見直したら全角って書いてました・・・!
image

たぶん中身が半角だと半角カッコの方がいいんだろうなと思います。
(ノーマル)(ノーマル)(Normal)(Normal)
(ノーマル)(ノーマル)(Normal)(Normal)

@thiramisu
Copy link
Contributor Author

thiramisu commented Aug 22, 2023

あるいは一旦タイトルの通り定数にする部分だけにするとかもありだと思います!

このコメントの通り、PR分けるのが良い気がしてきました!
定数化に関しては特に問題なさそうなのでそちらを新たに作成し、議論が進んでいるこちらの名前を「括弧の全角と半角を統一する」としようかなと思います。

一旦定数化のほうのPRのマージを待とうかなと…!

@thiramisu thiramisu changed the title スタイルの"ノーマル"を定数にする 括弧の全角と半角を統一する Aug 22, 2023
@thiramisu thiramisu changed the title 括弧の全角と半角を統一する UIの括弧の全角と半角を統一する Aug 22, 2023
@thiramisu thiramisu marked this pull request as draft August 22, 2023 17:37
@thiramisu thiramisu marked this pull request as ready for review August 26, 2023 09:47
@thiramisu
Copy link
Contributor Author

thiramisu commented Aug 26, 2023

全角括弧の方に統一できた&merge mainで無関係なテストが落ちなくなっていそうだったのでDraftを外しました。

#1510 (review)
「テキストを繋げて書き出し」は上記のコメントを参考に以下の仕様に変更しました。

  • 書き出しは全角括弧に変更
  • 読み込みは全角・半角の両方に対応

ちなみに念のための確認なのですが、以下の2箇所は元から括弧が無く、本PRでも追加していませんが、このままで大丈夫でしょうか。
括弧を付けた場合、若干見た目が悪くなるのと引き換えに分かりやすさは上がる気もしますが、まあ誤差な気もします。

現状
image
(試しに括弧ありにしてみたバージョン)
image

現状
image
(試しに括弧ありにしてみたバージョン)
image

@Hiroshiba
Copy link
Member

ちなみに念のための確認なのですが、以下の2箇所は元から括弧が無く、本PRでも追加していませんが、このままで大丈夫でしょうか。

このままで大丈夫だと思います!
キャラクター名の横にスタイル名を書く時はキャラ(スタイル)という感じかなと!

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です!!

ちなみにUI以外の部分も半角括弧が全角括弧に置き換わっていますが意図的でしょうか 👀

src/components/CharacterPortrait.vue Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba Aug 27, 2023

Choose a reason for hiding this comment

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

(ただのメモです)
issue templateはそういえば.githubリポジトリの方に移しちゃって、全VOICEVOXリポジトリに同じテンプレートが適用されるようにしていたんでした。
後で https://github.com/VOICEVOX/.github/tree/main/.github/ISSUE_TEMPLATE の方も変えておこうと思います。

@Hiroshiba
Copy link
Member

抜け漏れがないかどうかチェックしようと思ったのですが、関数利用の()などが大量に引っかかってかなり難しそうでした。。。
どのようにして検知されましたか・・・?

@thiramisu thiramisu changed the title UIの括弧の全角と半角を統一する UI・.mdファイルの括弧の全角と半角を統一する Aug 27, 2023
@thiramisu
Copy link
Contributor Author

thiramisu commented Aug 27, 2023

ちなみにUI以外の部分も半角括弧が全角括弧に置き換わっていますが意図的でしょうか 👀

範囲の想定としては「コメントアウト以外のすべての場所」という感じで、実質的に.mdファイルのみだったのでタイトルをそのように変更しました。


どのようにして検知されましたか・・・?

いい方法が思いつかなかったので割と人力です 😇
検索窓の横幅を増やしたり、キーボードの上下で前・次の候補に飛べるのを利用すれば少しだけ分かりやすくなるかもです…

269bf59
正規表現[^\]]\(.*[^ -#%-~]+.*\)で検索し一つずつ見ていった

@thiramisu
Copy link
Contributor Author

レビューありがとうございます!反映しました。

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!!

いい方法が思いつかなかったので割と人力です 😇
検索窓の横幅を増やしたり、キーボードの上下で前・次の候補に飛べるのを利用すれば少しだけ分かりやすくなるかもです…

なるほどです!!ありがとうございます!!


一箇所追加で見つけたので勝手ながら変更コミットをさせていただきました 🙇
(ChatGPTに教えてもらった\(([^)]*[^ -~][^)]*)\)で探しました)

@thiramisu
Copy link
Contributor Author

抜け修正ありがとうございます。助かります。

@thiramisu thiramisu deleted the define-default-style-name branch August 29, 2023 08: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.

2 participants