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

検索ページ用ユーザー一覧を作成して置換 #166

Merged
merged 5 commits into from
Jun 29, 2024

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented Jun 23, 2024

User description

検索結果で「チームメンバー」って文字があって、ContestTeamのコンポーネントを検索結果ページで使ってるのがよくなかったため、新しく別で作成


PR Type

Enhancement


Description

  • 検索ページ用の新しいユーザー一覧コンポーネントを作成し、既存のコンポーネントを置換しました。
  • ユーザーリストアイテムコンポーネントを新規作成し、ユーザーアイコンと名前を表示する機能を追加しました。
  • 検索ページで使用するメンバーリストコンポーネントをContestTeamからSearchに変更しました。

Changes walkthrough 📝

Relevant files
Enhancement
UserList.vue
検索ページ用のユーザー一覧コンポーネントを新規作成                                                               

src/components/Search/UserList.vue

  • 新しいユーザー一覧コンポーネントを作成
  • メンバーリストを表示するためのテンプレートとスタイルを追加
+36/-0   
UserListItem.vue
ユーザーリストアイテムコンポーネントを新規作成                                                                   

src/components/Search/UserListItem.vue

  • ユーザーアイコンと名前を表示するリストアイテムコンポーネントを作成
  • スタイルを追加
+30/-0   
SearchPage.vue
検索ページのメンバーリストコンポーネントを置換                                                                   

src/pages/SearchPage.vue

  • 検索ページで使用するメンバーリストコンポーネントを置換
+1/-1     

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

@mehm8128 mehm8128 requested a review from Pugma June 23, 2024 05:52
@mehm8128 mehm8128 self-assigned this Jun 23, 2024
@mehm8128 mehm8128 changed the title 検索ページ用ユーザー一覧を作成 検索ページ用ユーザー一覧を作成して置換 Jun 23, 2024
@Pugma
Copy link
Collaborator

Pugma commented Jun 27, 2024

/improve

Copy link

Failed to generate code suggestions for PR

@Pugma Pugma marked this pull request as draft June 27, 2024 07:37
@Pugma Pugma marked this pull request as ready for review June 27, 2024 07:37
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 2
🧪 Relevant tests No
🔒 Security concerns No
⚡ Key issues to review コンポーネントの命名:
MemberListItem という名前が UserListItem.vue でインポートされていますが、実際のファイル名と一致していません。これは混乱を招く可能性があります。
プロパティのバインディング:
UserListItem.vue コンポーネントに :user-id="member.name" とありますが、これはおそらく :user-id="member.id" の間違いではないでしょうか?名前ではなく、ユーザーIDを渡すべきです。

Copy link

Failed to generate code suggestions for PR

@Pugma
Copy link
Collaborator

Pugma commented Jun 27, 2024

/improve

Copy link

Failed to generate code suggestions for PR

@Pugma
Copy link
Collaborator

Pugma commented Jun 27, 2024

/improve --extended

Copy link

Failed to generate code suggestions for PR

@Pugma
Copy link
Collaborator

Pugma commented Jun 27, 2024

/improve

Copy link

Failed to generate code suggestions for PR

@mehm8128
Copy link
Contributor Author

/improve

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Rename the component in the template to match its file and import name for clarity

Consider using a more descriptive component name in the template for clarity and
consistency. Instead of , use to match the imported component name and file name.

src/components/Search/UserList.vue [15-18]

-<member-list-item
+<user-list-item
   v-for="member in members"
   :key="member.id"
   :member="member"
 />
 
Suggestion importance[1-10]: 9

Why: The suggestion improves code clarity and consistency by ensuring the component name in the template matches the imported component name and file name. This change reduces confusion and potential errors.

9
Accessibility
Improve accessibility by adding a title attribute to the router link

Add a title attribute to the to improve accessibility and provide additional context on
hover, which can be particularly useful for users who rely on assistive technologies.

src/components/Search/UserListItem.vue [12]

-<router-link :class="$style.link" :to="`/users/${member.id}`">
+<router-link :class="$style.link" :to="`/users/${member.id}`" :title="`View profile of ${member.name}`">
 
Suggestion importance[1-10]: 8

Why: Adding a title attribute enhances accessibility by providing additional context for users relying on assistive technologies, which is a significant improvement.

8
Maintainability
Use a computed property for constructing user profile URLs to enhance readability and manageability

Consider using a computed property for the user profile URL to simplify the template and
enhance readability. This change centralizes the logic for URL construction, making it
easier to manage and modify.

src/components/Search/UserListItem.vue [12]

-<router-link :class="$style.link" :to="`/users/${member.id}`">
+<router-link :class="$style.link" :to="userProfileUrl">
 
Suggestion importance[1-10]: 7

Why: Using a computed property centralizes the URL construction logic, improving readability and maintainability. However, it is a minor enhancement rather than a critical fix.

7
Use a CSS variable for text color to ensure consistency with theme changes

To ensure that the text color adapts dynamically to theme changes, consider using a CSS
variable for the text color in the .name class. This approach promotes better theme
consistency and easier maintenance.

src/components/Search/UserListItem.vue [26-27]

 .name {
-  color: $color-text;
+  color: var(--text-color);
 }
 
Suggestion importance[1-10]: 6

Why: Using a CSS variable for the text color improves maintainability and ensures the text color adapts to theme changes. This is a useful but minor improvement.

6

Copy link
Collaborator

@Pugma Pugma left a comment

Choose a reason for hiding this comment

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

ありがとうございます
いいと思うのでこのままマージしちゃってもいいですが、一点だけ確認をお願いします!

</script>

<template>
<div :class="$style.container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

member-listの方はこういう表記になっていたんですが、ここでタグを変更した理由はありますか…?

Suggested change
<div :class="$style.container">
<section :class="$style.container">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

基本的にsectionタグの中(の一番上)には見出しを置いた方がいいのですが、今回特に置く見出しがなかった(すぐ上にh1があったし)のでsectionじゃなくて単純にdivにしたって感じです

Copy link
Collaborator

Choose a reason for hiding this comment

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

:haakusimasita:
じゃあこれでマージしちゃって大丈夫です!

@mehm8128 mehm8128 merged commit 52e859a into master Jun 29, 2024
8 checks passed
@mehm8128 mehm8128 deleted the feat/search_user_list branch June 29, 2024 08:53
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