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

feat/グループ詳細のレイアウトを Figma に揃える #294

Merged
merged 6 commits into from
Sep 24, 2024

Conversation

anko9801
Copy link
Contributor

@anko9801 anko9801 commented Sep 14, 2024

User description

fix: #165


PR Type

Enhancement


Description

  • グループ詳細ページのレイアウトを簡素化し、Figmaに合わせました。
  • 編集モードを削除し、表示をシンプルにしました。
  • メンバーとオーナーのリスト表示を改善し、権限チェックを追加しました。
  • 不要なインポートとコンポーネントを削除しました。

Changes walkthrough 📝

Relevant files
Enhancement
GroupBudget.vue
Simplify Group Budget Display                                                       

src/components/groupDetail/GroupBudget.vue

  • Removed edit mode and related components.
  • Simplified budget display layout.
  • +4/-54   
    GroupDescription.vue
    Simplify Group Description Display                                             

    src/components/groupDetail/GroupDescription.vue

  • Removed edit mode and related components.
  • Simplified description display layout.
  • +4/-60   
    GroupMembers.vue
    Simplify Group Members Display and Authority Check             

    src/components/groupDetail/GroupMembers.vue

  • Simplified member list layout.
  • Added authority check for member removal.
  • +32/-34 
    GroupName.vue
    Simplify Group Name Editing                                                           

    src/components/groupDetail/GroupName.vue

  • Replaced SimpleButton with EditButton.
  • Simplified group name editing logic.
  • +9/-29   
    GroupOwners.vue
    Simplify Group Owners Display and Authority Check               

    src/components/groupDetail/GroupOwners.vue

  • Simplified owner list layout.
  • Added authority check for owner removal.
  • +31/-33 
    InputSelectMultiple.vue
    Refactor InputSelectMultiple Component                                     

    src/components/shared/InputSelectMultiple.vue

  • Adjusted class binding for styling.
  • Minor refactoring for readability.
  • +5/-7     
    GroupDetailPage.vue
    Simplify Group Detail Page Layout                                               

    src/pages/GroupDetailPage.vue

  • Removed unnecessary imports and components.
  • Simplified layout and component usage.
  • +6/-32   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented Sep 14, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 876701a)

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    @reiroop
    Copy link
    Contributor

    reiroop commented Sep 14, 2024

    /review

    @reiroop
    Copy link
    Contributor

    reiroop commented Sep 14, 2024

    /improve

    Copy link

    Persistent review updated to latest commit 876701a

    Copy link

    github-actions bot commented Sep 14, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    v-forディレクティブのキーとして一意のプロパティを使用する。


    v-forディレクティブ内で使用されるmemberのキーとしてmember自体を使用していますが、これが一意の値であることを保証するためには、member.idのような一意のプロパティを使用することをお勧めします。これにより、VueがDOMを効率的に再利用し、更新することができます。

    src/components/groupDetail/GroupMembers.vue [31-33]

     <li
       v-for="member in group.members"
    -  :key="member"
    +  :key="member.id"
       class="flex items-center justify-between">
     
    Suggestion importance[1-10]: 9

    Why: Using a unique key like member.id in v-for ensures efficient DOM updates and prevents potential rendering issues, which is a best practice in Vue.js development.

    9
    Enhancement
    groupが存在しない場合の表示を明示的に管理する。


    group?.budgetの使用により、groupnullまたはundefinedの場合に安全にプロパティにアクセスできますが、groupが存在しない場合の表示を明示的に管理することをお勧めします。例えば、予算が未設定の場合に「未設定」と表示するなどの処理を追加することが考えられます。

    src/components/groupDetail/GroupBudget.vue [14]

    -<p class="mt-3">{{ group?.budget }} 円</p>
    +<p class="mt-3">{{ group ? `${group.budget} 円` : '予算未設定' }}</p>
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves user experience by providing a clear indication when the budget is not set, which is a meaningful enhancement for users viewing the budget information.

    8
    User experience
    権限がない場合のユーザーインターフェースを改善する。


    v-if="hasAuthority"を使用している箇所で、権限がない場合のUI表示を考慮していないようです。権限がない場合にもユーザーが適切なフィードバックを受けられるように、UIの調整を行うことをお勧めします。例えば、権限がない場合にはボタンを非表示にするのではなく、無効化して説明を加えるなどが考えられます。

    src/components/groupDetail/GroupOwners.vue [39-43]

     <button
    -  v-if="hasAuthority"
       class="flex items-center rounded-full p-1 hover:bg-surface-secondary"
    -  :disabled="isSending"
    -  @click="removeOwner(owner)">
    +  :disabled="isSending || !hasAuthority"
    +  @click="hasAuthority ? removeOwner(owner) : null">
       <MinusIcon class="w-6" />
    +  <span v-if="!hasAuthority" class="ml-2 text-sm text-gray-500">権限がありません</span>
     </button>
     
    Suggestion importance[1-10]: 7

    Why: Enhancing the UI to provide feedback when a user lacks authority improves user experience by making the interface more informative and user-friendly.

    7
    Maintainability
    EditButtonの編集モードの状態管理をシンプルにする。


    EditButtonコンポーネントの:is-edit-modeプロパティにprops.isEditModeをそのまま渡していますが、これによりEditButton内部での状態管理が複雑になる可能性があります。EditButton内部で編集モードの状態を管理するのではなく、外部から状態を渡す設計にすることを検討してください。

    src/components/groupDetail/GroupName.vue [42-49]

     <EditButton
       v-if="hasAuthority"
    -  :is-edit-mode="props.isEditMode"
       @click="
         props.isEditMode
           ? emit('finishEditing')
           : emit('changeEditMode', 'name')
       " />
     
    Suggestion importance[1-10]: 5

    Why: While simplifying the state management of EditButton could improve maintainability, the suggestion does not address a critical issue and may not significantly impact the current implementation.

    5

    Copy link
    Contributor

    @reiroop reiroop left a comment

    Choose a reason for hiding this comment

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

    編集などの traPtitech/Jomon#561 に関わる部分はあとで変更の可能性があることを明示的に残しておきたい気持ちがあります

    src/pages/GroupDetailPage.vue Show resolved Hide resolved
    src/components/groupDetail/GroupName.vue Outdated Show resolved Hide resolved
    @reiroop reiroop self-requested a review September 14, 2024 07:50
    Copy link
    Contributor

    @reiroop reiroop left a comment

    Choose a reason for hiding this comment

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

    めちゃめちゃ綺麗にレスポンシブ対応されてて感動しました

    @reiroop
    Copy link
    Contributor

    reiroop commented Sep 22, 2024

    あ、ごめんなさい、コンフリクトのほうだけお願いします!

    @anko9801 anko9801 merged commit 9163d90 into main Sep 24, 2024
    6 checks passed
    @anko9801 anko9801 deleted the feat/group_detail branch September 24, 2024 12:54
    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.

    グループ詳細ページ
    2 participants