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

プロジェクトメンバーを加入日昇順にソート #255

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

ogu-kazemiya
Copy link
Contributor

@ogu-kazemiya ogu-kazemiya commented Dec 5, 2024

User description

close #224


PR Type

Enhancement


Description

  • プロジェクトメンバーを加入日昇順にソートするためのロジックを追加。
  • computedプロパティsortedMembersを作成し、yearsemesterを基準にメンバーをソート。
  • テンプレート内でv-forディレクティブをsortedMembersに変更し、ソートされたリストを表示。

Changes walkthrough 📝

Relevant files
Enhancement
MemberList.vue
メンバーリストを加入日昇順にソートするロジックを追加                                                             

src/components/Project/MemberList.vue

  • 新しいcomputedプロパティsortedMembersを追加し、メンバーを加入日昇順にソート。
  • v-forディレクティブでmembersの代わりにsortedMembersを使用。
  • メンバーのソートロジックをyearsemesterで実装。
  • +16/-2   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Dec 5, 2024

    Copy link

    github-actions bot commented Dec 5, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    224 - Partially compliant

    Fully compliant requirements:

    • メンバーを加入順に並べる

    Not compliant requirements:

    • 現状参加しているメンバー(until=未定)は上位に並ぶ
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Sorting Logic
    メンバーのソートロジックが年と学期のみを考慮しており、現在参加中のメンバーを優先する要件が考慮されていない

    Copy link

    github-actions bot commented Dec 5, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    props.members の元の配列を変更しないように、ソート前に配列をコピーする。

    sortedMembers の計算プロパティ内で、配列 li を直接ソートすると、元の props.members
    配列も変更されてしまいます。これを防ぐために、ソート前に配列をコピーしてください。

    src/components/Project/MemberList.vue [12-23]

     const sortedMembers = computed(() => {
       if (!props.members) return []
    -  const li = props.members
    +  const li = [...props.members]
       li.sort((a, b) => {
         if (a.duration.since.year !== b.duration.since.year) {
           return a.duration.since.year - b.duration.since.year
         } else {
           return a.duration.since.semester - b.duration.since.semester
         }
       })
       return li
     })
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly identifies a potential issue where sorting the array directly affects the original props.members array. The suggested use of array spreading to create a shallow copy before sorting is a best practice in JavaScript to avoid unintended side effects. This change is crucial for maintaining the integrity of the original data structure.

    9

    Copy link
    Contributor

    @Futadaruma Futadaruma left a comment

    Choose a reason for hiding this comment

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

    参加開始年→学期(前期/後期)に従ってソートされるようになってて良さそうです
    また、dashboard側で参加期間の入力は必須になっているので、参加期間未登録の場合の動作は考えなくて良いようになっていると思います
    ありがとうございます!

    @ogu-kazemiya ogu-kazemiya merged commit fd184dc into main Dec 5, 2024
    12 checks passed
    @ogu-kazemiya ogu-kazemiya deleted the feat/sort-members branch December 5, 2024 14:01
    Comment on lines +15 to +16
    const li = props.members
    li.sort((a, b) => {
    Copy link
    Collaborator

    @Pugma Pugma Dec 9, 2024

    Choose a reason for hiding this comment

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

    ここで配列をコピーしてから改めて sort 関数をかけているけど、 toSorted 関数を利用するともっと便利にできたかもです
    これでも動作は問題ないので書き換える必要まではないと思うんだけど、こういうのもあると知っておくといいかも
    この関数は Dashboard のリポジトリで使われてました

    参考リンク
    MDN - Array.prototype.toSorted()

    Comment on lines +15 to +16
    const li = props.members
    li.sort((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.

    外部から失礼します
    JS で const x = someArray のように書くと、参照する配列が同じものとなってしまうため、この処理だと li をソートするのと同時に、props.members もソートされてしまいます

    特にこの場合だと props の値を変更していて、親要素側の値も変更されてしまうので予期せぬバグ (特にリアクティブ性が失われる変更方法で、バグによくわからないタイミングで更新されたように見えたりする) を引き起こしやすいです
    (ref: https://ja.vuejs.org/guide/components/props#mutating-object-array-props)
    なので、スプレッド構文などを使った浅いコピーを行ってからソートするか、新し目の機能になってしまうのですが Pugma くんが言ってるような toSorted を使うのがよさそうです

    const li = [...props.members]
    
    or
    
    const li = props.members.toSorted(...)

    参考コード: TypeScript Playground

    @ogu-kazemiya
    Copy link
    Contributor Author

    ありがとうございます!!
    仕様確認して修正します

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    メンバーを加入順で並べる
    4 participants