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

/projects/:projectId/members #58

Merged
merged 14 commits into from
Jan 4, 2024
Merged

Conversation

tesso57
Copy link
Member

@tesso57 tesso57 commented Jan 4, 2023

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 4, 2023

多分Figmaの時点でちょっとおかしかったと思うんですけど、このページってprojectの修正ではなくて新規作成のときのメンバー入力画面ですよね?(修正は他の情報と一緒に1つの画面でできるようになっていそう)
だとするとプロジェクトの情報の取得とかそれを使う部分とかいらなくなりそうなので、そこらへん修正してほしいです🙏

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 8, 2023

あとFigmaだとそれぞれのメンバーの在籍期間を入力するフォームがない気がするのでどこかに入れたいですね

@tesso57 tesso57 self-assigned this Mar 8, 2023
@mehm8128
Copy link
Contributor

mehm8128 commented Aug 7, 2023

@mehm8128
Copy link
Contributor

そういえば在籍期間フォームのデザインもなんとなくで作りました

@tesso57 tesso57 requested a review from mehm8128 October 31, 2023 13:57
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

細かいところ見れてないですが一旦
確認お願いします:pray:

src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
src/pages/ProjectMembers.vue Outdated Show resolved Hide resolved
src/components/UI/FormProjectDuration.vue Outdated Show resolved Hide resolved
src/components/Projects/ProjectMember.vue Outdated Show resolved Hide resolved
src/components/Projects/ProjectMember.vue Show resolved Hide resolved
src/components/UI/FormProjectDuration.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

1個見逃してました

src/components/Projects/ProjectMember.vue Outdated Show resolved Hide resolved
@tesso57 tesso57 requested a review from mehm8128 November 6, 2023 08:12
@tesso57
Copy link
Member Author

tesso57 commented Nov 6, 2023

再度レビューお願いします。
相談なんですが、user情報とduration情報を一緒にするために、MemberInputをGenericを利用して解決しました。
Generic にすると、複雑さが増して無理やり共通化しているから良くないかなって少し迷ってます。

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

いくつかお願いします:pray:

Generic にすると、複雑さが増して無理やり共通化しているから良くないかな

そこまで無理やりな共通化だとは感じなかったので大丈夫だと思います:blob_thumbs_up:

src/pages/ProjectNew.vue Show resolved Hide resolved
src/use/validate.ts Show resolved Hide resolved
src/use/validate.ts Outdated Show resolved Hide resolved
src/use/validate.ts Outdated Show resolved Hide resolved
src/components/Projects/ProjectMember.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Outdated Show resolved Hide resolved
src/pages/ProjectNew.vue Show resolved Hide resolved
@tesso57
Copy link
Member Author

tesso57 commented Nov 21, 2023

再レビューお願いします。:pray:
ここらへんの実装がいい感じにできなくて困ってます。
一度、grid で実装したんですが、表示幅が足りないと要素が飛び出てしまいやめました。今の実装だと、widthを30%しているので微妙に感じています。

@tesso57 tesso57 requested a review from mehm8128 November 21, 2023 12:34
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

ありがとうございます、思ってたより複雑な感じになってしまいましたね:thonk_sweat:

一度、grid で実装したんですが、表示幅が足りないと要素が飛び出てしまいやめました。今の実装だと、widthを30%しているので微妙に感じています。

は一旦このままで大丈夫だと思います:pray:
最後に1箇所だけお願いしたいです

Comment on lines 29 to 35
const isEquals = (a: T, b: T) => {
if (value.value !== null && typeof value.value === 'object') {
return JSON.stringify(a) === JSON.stringify(b)
} else {
return a === b
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

imo
他のところでも使えそうなので、lib/deepFreeze.tsみたいに切り出しちゃってよさそうです

Copy link
Member

Choose a reason for hiding this comment

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

今回実装した部分では options からのみ modelValue が生成されてるので問題なさそうなんですが、stringify でオブジェクトを比較する方法は プロパティ の順番を変えたりするだけで等価でなくなってしまうので、そこは留意したほうが良さそうです
(特に util として切り出す場合)
ref: Playground

対策としては

  1. オブジェクトを再帰的に等しいか比較する
    正直めんどくさいので https://github.com/NickGard/tiny-isequal とかのライブラリに頼ったほうがいいと思います (このライブラリがベストかはわからないです)
  2. オブジェクトの特定の比較可能なプロパティを使って比較するようにする
    この場合は optional な Props として compareWith とか by とかの keyof T or (T) => any or (T, T) => boolean (もしくはこれらの和集合) をとって、指定されていない場合は object の id、指定されている場合はこれらから算出される値をつかって比較することになります
    https://headlessui.com/react/listbox#binding-objects-as-values とかはこの方式でした

とかがありそうです (個人的には 2 のほうが好みです)

一応 BaseSelect は今回いじらないことにして、src/components/UI/FormProjectDuration.vue で BaseSelect には一意な key のみを渡して、key からオブジェクトを算出するようにする方針もあります

:user="member"
:class="$style.projectMember"
@delete="handleDelete"
@update="member.duration = $event"
Copy link
Member

Choose a reason for hiding this comment

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

意図してたら申し訳ないのですが、ここ下のイベントを update:modelValue で発行するようにしたら @update を丸々消せませんか?

dateType === 'sinceSemester'
? numValue
: props.modelValue.since.semester
.map((_, i) => [
Copy link
Member

Choose a reason for hiding this comment

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

一応 .map して .flat するのは .flatMap でかけます 🤟
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/flatMap

Comment on lines 29 to 35
const isEquals = (a: T, b: T) => {
if (value.value !== null && typeof value.value === 'object') {
return JSON.stringify(a) === JSON.stringify(b)
} else {
return a === b
}
}
Copy link
Member

Choose a reason for hiding this comment

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

今回実装した部分では options からのみ modelValue が生成されてるので問題なさそうなんですが、stringify でオブジェクトを比較する方法は プロパティ の順番を変えたりするだけで等価でなくなってしまうので、そこは留意したほうが良さそうです
(特に util として切り出す場合)
ref: Playground

対策としては

  1. オブジェクトを再帰的に等しいか比較する
    正直めんどくさいので https://github.com/NickGard/tiny-isequal とかのライブラリに頼ったほうがいいと思います (このライブラリがベストかはわからないです)
  2. オブジェクトの特定の比較可能なプロパティを使って比較するようにする
    この場合は optional な Props として compareWith とか by とかの keyof T or (T) => any or (T, T) => boolean (もしくはこれらの和集合) をとって、指定されていない場合は object の id、指定されている場合はこれらから算出される値をつかって比較することになります
    https://headlessui.com/react/listbox#binding-objects-as-values とかはこの方式でした

とかがありそうです (個人的には 2 のほうが好みです)

一応 BaseSelect は今回いじらないことにして、src/components/UI/FormProjectDuration.vue で BaseSelect には一意な key のみを渡して、key からオブジェクトを算出するようにする方針もあります

@mehm8128 mehm8128 requested a review from SSlime-s December 26, 2023 12:38
@mehm8128
Copy link
Contributor

@SSlime-s レビューお願いしたいです:pray:

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

見た感じよさそうな気がするのでマージします
1年間ありがとうございました

@mehm8128 mehm8128 merged commit 1134b37 into main Jan 4, 2024
5 checks passed
@mehm8128 mehm8128 deleted the feat/projects/project_id/member branch January 4, 2024 09:35
@mehm8128 mehm8128 linked an issue Jan 8, 2024 that may be closed by this pull request
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.

FormProjectDurationの構造を変える /project/:project_id/member
3 participants