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: AudioInfoのパラメータ一覧周りをリファクタ #1322

Merged

Conversation

sevenc-nanashi
Copy link
Member

内容

タイトル通り。

関連 Issue

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

image

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner May 23, 2023 09:26
@sevenc-nanashi sevenc-nanashi requested review from y-chan and removed request for a team May 23, 2023 09:26
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.

@y-chan こちらのプルリクエストに行って一度見た内容なのでassignを僕に変更したいと思います!

@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan May 25, 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
@@ -1179,6 +966,13 @@ const adjustSliderValue = (
overflow-y: scroll;
}

.parameters {
Copy link
Member

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
@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

Comment on lines +809 to +817
...parameters.value.reduce(
(acc, parameter) => ({
...acc,
[parameter.key]: parameter.slider.state.currentValue.value,
}),
{} as {
[K in typeof parameters.value[number]["key"]]: number;
}
),
Copy link
Member

Choose a reason for hiding this comment

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

ここの型パズル、自分も挑戦してみたのですが難しいですね。。
今の形はparameter.slider.state.currentValue.valueがnullだとしても通っちゃいそうです。(まあ真上でthrowしているので今のコードは大丈夫なのですが)

真上のnull時throwと混ぜてこんな感じにするのもありかなとか思いました。だいぶ読みづらいですが、まあより正しいはずではある・・・ 😇

Suggested change
...parameters.value.reduce(
(acc, parameter) => ({
...acc,
[parameter.key]: parameter.slider.state.currentValue.value,
}),
{} as {
[K in typeof parameters.value[number]["key"]]: number;
}
),
...parameters.value.reduce(
(acc, parameter) => {
if (parameter.slider.state.currentValue.value == undefined)
throw new Error(
`parameter(${parameter.key}).slider.state.currentValue.value == undefined`
);
acc[parameter.key] = parameter.slider.state.currentValue.value;
return acc;
},
{} as {
[K in typeof parameters.value[number]["key"]]: number;
}
),

Copy link
Member Author

Choose a reason for hiding this comment

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

真上でthrowしてるので、わざわざ書く必要もないかなぁって思ってます

Copy link
Member

Choose a reason for hiding this comment

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

真上でthrowしてるのを下に統合しちゃうと型サポートも入って良い感じって意図でした。
TypeScriptの型サポートが中途半端なんすよね・・・

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

2点ほどsuggestしているので、気に入ったのあればよかったらどうぞくらいです 🙏

@Hiroshiba
Copy link
Member

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

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