[Improve code]: components/Client/common/* 手動formatter・Linter対応 #94

ZEKE320 · 2024-04-13T10:36:05Z

Resolves #59
以下の対応を行いました。

1064677 src/components/Client/common/*にPrettierを適用
5982e58 セレクトボックス共通実装にて、codeMapにジェネリック型を導入

…ssue_59

ZEKE320 · 2024-04-13T11:33:08Z

@nkte8
実装について意見をお伺いしたいです

現状では投稿の言語を設定する際に、codeに設定されている"ja" "en"などの文字列を用いていると思います

skyshare/astro/src/components/Client/bsky/selectLists/LanguageSelectList.tsx

Lines 3 to 40 in 1284b22

    
           const langList = [ 
        
               { 
        
                   label: "日本語", 
        
                   code: "ja", 
        
               }, 
        
               { 
        
                   label: "English", 
        
                   code: "en", 
        
               }, 
        
               { 
        
                   label: "中文", 
        
                   code: "zh", 
        
               }, 
        
               { 
        
                   label: "한국어", 
        
                   code: "ko", 
        
               }, 
        
               { 
        
                   label: "Polski", 
        
                   code: "pl", 
        
               }, 
        
               { 
        
                   label: "Portuguese", 
        
                   code: "pt", 
        
               }, 
        
               { 
        
                   label: "Deutsch", 
        
                   code: "de", 
        
               }, 
        
               { 
        
                   label: "Français", 
        
                   code: "fr", 
        
               }, 
        
               { 
        
                   label: "हिन्दी", 
        
                   code: "hi", 
        
               }, 
        
           ]

また投稿時に用いられるPostButton.tsxでは、上記codeに相当する変数languageがstring型であると定義されています

skyshare/astro/src/components/Client/bsky/buttons/PostButton.tsx

Line 49 in 1284b22

language: string

一方で言語選択で用いられている共通実装では、setLanguage()に相当するsetCode()にnullがセットできるような仕様となっています

skyshare/astro/src/components/Client/common/SelectList.tsx

Lines 17 to 22 in 1284b22

    
           let value: any = null 
        
           const item = codeMap.find((opt) => opt.label === event.target.value) 
        
           if (typeof item !== "undefined") { 
        
               value = item.code 
        
           } 
        
           setCode(value)

今回の対応では、この部分の実装を

nullが許容できない場合は、デフォルト値（言語設定であれば"ja"）をセットする
codeMap: codeMap[]の１番目のcodeをセットする

といったものに変更する必要がありそうです
どちらの方法で対応しましょうか？

（追記 2024/04/13）
当面の対応として、codeMap[0].codeをデフォルト値として用いる方針で決定しました

cloudflare-workers-and-pages · 2024-04-13T13:00:38Z

Deploying skyshare with Cloudflare Pages

Latest commit:	`d44cc71`
Status:	✅ Deploy successful!
Preview URL:	https://13823ba5.skyshare.pages.dev
Branch Preview URL:	https://preview-issue-59.skyshare.pages.dev

View logs

nkte8 · 2024-04-13T13:01:09Z

@ZEKE320 対応ありがとうございます！
本リポジトリを閲覧する開発者に向けて、内部Discordでの結論をまとめておきます

nullをなぜ許容できる書き方をしているのか
langパラメータは任意なので、空にしてもいいのだけれど、現在のSelectboxではその指定ができない状態になってしまっている
ねこののWillとして思っているのが

i18n対応により、初回で開いたlangのページをlocalstorageに保存

localstorageを参照してデフォルト値をセット

要はlanguage: nullは許容したい、という話が展開されました。
このディスカッションにより以下の方針を取っていただくということで、（対処はi18n対応が進むことで変更され、本変更が一時的なものになる/よりよい形に修正される可能性がある前提で）ねこのが担当者にOKを出しました

未選択時はcodeMapの０番目を選択するような実装で、今回は解決できそうです
ラベルの0番目がnull、言語の0番目が"ja"なので、これでも問題は無いのかなと
...(以下コード)...

選択としては

codeMap: codeMap[]の１番目のcodeをセットする

を踏襲する決定になります。

蛇足:
このコメントの際にコードを見直した際、 #95 の課題を発見したため、Issueを起票しました

ZEKE320 · 2024-04-13T13:07:53Z

@nkte8
下記二点対応を行いました。レビューをお願いします！

1064677 src/components/Client/common/*にPrettierを適用
5982e58 セレクトボックス共通実装にて、codeMapにジェネリック型を導入

nkte8

対応ありがとうございます！方針自体はOKです！コメントの追加（おまかせしますがコードの改善コメントの対応）をお願いします

nkte8 · 2024-04-13T14:34:49Z

astro/src/components/Client/common/SelectList.tsx

+        const item: CodeMap<CodeType> | undefined = codeMap.find(
+            opt => opt.label === event.target.value,
+        )
+        const code: CodeType | undefined = item?.code ?? codeMap[0]?.code


方針自体はDiscordで可決した通りOKです！

内容へのコメントとして、この一行の情報量が(typescript初心者的な意見ですが)すこし多い気がするので、コメントで補足をしていただきたいです。以下は例です

// 選択項目の`code`パラメータが`undefined`または`null`の場合に、codeMapの最初の項を採用する const code: CodeType | undefined = item?.code ?? codeMap[0]?.code

あとは...なんとなくですがcodeMap[0]?.codeがあまり好ましくないので、この判定の前にcodeMapのlengthが0以上でなかったらreturnしちゃってもいいかもしれませんね。おまかせします。

今気がついたのですが、現状ではnullも正常値として扱われているということでしたね

となると??で判定するのは正しくないので、

// `item`が`undefined`の場合に、codeMapの最初の項を採用する。 const code: CodeType = item === undefined ? codeMap[0].code : item.code setCode(code)

冗長な書き方とはなってしまいますが、こちらの実装の方が良いでしょうか？

現状ではnullも正常値として扱われているということでしたね

そうでした、気づいていただきありがとうございます！
ぜひそっちの書き方でおねがいします！

対応行いました！

…ssue_59

ZEKE320 · 2024-04-13T16:36:50Z

@nkte8
下記2点対応行いました
改めてレビューをお願いします

bb4bbbc codeMap: CodeMap[]が空の場合の早期リターン追加
3299ddd itemがundefinedの場合だけ、codeMap[0]を用いる実装に変更 & コメント追加

nkte8

対応ありがとうございました！マージします。
Discordでも記載していただいていたように、現在nullとなっている箇所は紛らわしさの解消の意味で、いずれはundefinedで置き換えていきたいですね。

…対応 (#94)" This reverts commit a12296d.

ZEKE320 added 2 commits April 12, 2024 15:57

🎨 src/components/Client/common/* にPrettierを適用

1064677

Merge remote-tracking branch 'origin/feature/issue_59' into preview/i…

e5d1db3

…ssue_59

ZEKE320 added this to the [Improve code] 手動Linter/Formatter対応 milestone Apr 13, 2024

ZEKE320 self-assigned this Apr 13, 2024

ZEKE320 added 4 commits April 13, 2024 11:45

Merge remote-tracking branch 'upstream/develop' into feature/issue_59

6ae5a74

🏷️ CodeMapのcodeをジェネリック型で定義し、共通実装として複数の型で利用できるように変更

5982e58

Merge remote-tracking branch 'upstream/develop' into preview/issue_59

d06c6f9

Merge branch 'feature/issue_59' into preview/issue_59

9962ab9

ZEKE320 linked an issue Apr 13, 2024 that may be closed by this pull request

[Improve code]: components/Client/common/* 手動formatter・Linter対応 #59

Closed

13 tasks

ZEKE320 requested a review from nkte8 April 13, 2024 13:04

ZEKE320 marked this pull request as ready for review April 13, 2024 13:04

nkte8 requested changes Apr 13, 2024

View reviewed changes

ZEKE320 added 3 commits April 13, 2024 15:41

🦺 codeMapが空の場合の早期リターンを追加

bb4bbbc

♻️ itemがundefinedの場合にデフォルト値を設定するよう、処理を変更

3299ddd

Merge remote-tracking branch 'origin/feature/issue_59' into preview/i…

d44cc71

…ssue_59

ZEKE320 requested a review from nkte8 April 13, 2024 16:34

nkte8 approved these changes Apr 13, 2024

View reviewed changes

nkte8 deployed to preview April 13, 2024 16:54 — with GitHub Actions View deployment

nkte8 merged commit a12296d into develop Apr 13, 2024
4 checks passed

nkte8 deleted the preview/issue_59 branch April 13, 2024 16:57

nkte8 added a commit that referenced this pull request Apr 14, 2024

Revert "[Improve code]: components/Client/common/* 手動formatter・Linter…

59c8b8e

…対応 (#94)" This reverts commit a12296d.

nkte8 pushed a commit that referenced this pull request Apr 14, 2024

[Improve code]: components/Client/common/* 手動formatter・Linter対応 (#94)

8f612c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improve code]: components/Client/common/* 手動formatter・Linter対応 #94

[Improve code]: components/Client/common/* 手動formatter・Linter対応 #94

ZEKE320 commented Apr 13, 2024 •

edited

Loading

ZEKE320 commented Apr 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 13, 2024 •

edited

Loading

nkte8 commented Apr 13, 2024 •

edited

Loading

ZEKE320 commented Apr 13, 2024

nkte8 left a comment

nkte8 Apr 13, 2024

ZEKE320 Apr 13, 2024 •

edited

Loading

nkte8 Apr 13, 2024

ZEKE320 Apr 13, 2024

ZEKE320 commented Apr 13, 2024 •

edited

Loading

nkte8 left a comment

[Improve code]: components/Client/common/* 手動formatter・Linter対応 #94

[Improve code]: components/Client/common/* 手動formatter・Linter対応 #94

Conversation

ZEKE320 commented Apr 13, 2024 • edited Loading

ZEKE320 commented Apr 13, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Apr 13, 2024 • edited Loading

Deploying skyshare with Cloudflare Pages

nkte8 commented Apr 13, 2024 • edited Loading

ZEKE320 commented Apr 13, 2024

nkte8 left a comment

Choose a reason for hiding this comment

nkte8 Apr 13, 2024

Choose a reason for hiding this comment

ZEKE320 Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

nkte8 Apr 13, 2024

Choose a reason for hiding this comment

ZEKE320 Apr 13, 2024

Choose a reason for hiding this comment

ZEKE320 commented Apr 13, 2024 • edited Loading

nkte8 left a comment

Choose a reason for hiding this comment

ZEKE320 commented Apr 13, 2024 •

edited

Loading

ZEKE320 commented Apr 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 13, 2024 •

edited

Loading

nkte8 commented Apr 13, 2024 •

edited

Loading

ZEKE320 Apr 13, 2024 •

edited

Loading

ZEKE320 commented Apr 13, 2024 •

edited

Loading