-
Notifications
You must be signed in to change notification settings - Fork 40
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
Modalの分岐にObjectを使うようにした #4325
Modalの分岐にObjectを使うようにした #4325
Conversation
Preview (prod) → https://4325-prod.traq-preview.trapti.tech/ |
@ZOI-dayo CI落ちてるのでちょっとチェックしますね |
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.
コード自体はちゃんと書けてそうでGoodです!
ちょっとだけコメントしたので確認お願いします!
return GroupMemberAddModal | ||
case 'settings-theme-edit': | ||
return SettingsThemeEditModal | ||
if (currentState.value && currentState.value.type in components) { |
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.
should: in
演算子は結構怪しくて、これをちゃんと書くなら Object.hasOwn(components, currentState.value.type)
と書くのが良い気がします
↑の例だと'toString' in {}
は true
です
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.
imo: そもそも型安全性が確保されているのでわざわざチェックする必要はないような気もします
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.
inを消してオブジェクト内に存在するかのチェックを切ってますが、cpくんが言ってくれている通りすでにtype
の値が存在する場合はModalStateValue
に含まれているので問題なさそうですね
okです!
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.
良さそうです!
ありがとうございます!!
close #4189