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/admin ページのレイアウトを Figma に揃える #271

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

anko9801
Copy link
Contributor

@anko9801 anko9801 commented Aug 28, 2024

User description

fix: #163


PR Type

Enhancement


Description

  • 管理者ページのレイアウトをFigmaに合わせて更新しました。
  • 管理者の追加と削除のUIを改善し、操作をより直感的にしました。
  • タグ削除機能のUIを改善し、ユーザーエクスペリエンスを向上させました。

Changes walkthrough 📝

Relevant files
Enhancement
AdminPage.vue
管理者ページのUIとレイアウトの改善                                                                             

src/pages/AdminPage.vue

  • 管理者ページのレイアウトを更新
  • 管理者の追加と削除のUIを改善
  • タグ削除機能のUIを改善
+52/-46 

💡 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 Aug 28, 2024

PR Reviewer Guide 🔍

(Review updated until commit a0904fe)

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

コードの整合性
props.options?.length ?? 0 の記述が (props.options?.length ?? 0) に変更されていますが、この変更は意図的なものでしょうか?括弧の追加は特に必要ないように見えますが、意図的な変更であればその理由を明確にする必要があります。

UIコンポーネントの整合性
新しいUIの構造とスタイルが既存のデザインガイドラインや他のページと整合性が取れているか確認が必要です。特に、InputSelectMultipleSimpleButton の使用方法が以前のコードから大きく変更されています。

@reiroop
Copy link
Contributor

reiroop commented Sep 7, 2024

/review

@reiroop
Copy link
Contributor

reiroop commented Sep 7, 2024

/improve

Copy link

github-actions bot commented Sep 7, 2024

Persistent review updated to latest commit f6dc652

@reiroop
Copy link
Contributor

reiroop commented Sep 8, 2024

/review

@reiroop
Copy link
Contributor

reiroop commented Sep 8, 2024

/improve

Copy link

github-actions bot commented Sep 8, 2024

Persistent review updated to latest commit a0904fe

Copy link

github-actions bot commented Sep 8, 2024

PR Code Suggestions ✨

Latest suggestions up to a0904fe

CategorySuggestion                                                                                                                                    Score
Possible bug
props.options が未定義の場合に lengthundefined になるのを防ぐための修正です。

props.options?.length ?? 0 の部分を props.options?.length || 0
に変更してください。これにより、props.optionsundefined または null の場合に 0 が返され、lengthundefined
になる可能性を排除できます。

src/components/shared/InputSelectMultiple.vue [184]

-: (props.options?.length ?? 0)
+: (props.options?.length || 0)
 
Suggestion importance[1-10]: 8

Why: The suggestion correctly addresses a potential issue where props.options could be undefined or null, which would result in length being undefined. By using || 0, it ensures that 0 is returned in such cases, improving the robustness of the code.

8

Previous suggestions

Suggestions up to commit f6dc652
CategorySuggestion                                                                                                                                    Score
Possible bug
length が 0 の場合に割り算をスキップするように修正してください。

props.optionsnull または undefined の場合、length が 0 になるため、focusingListItemIndex.value
の計算で割り算によるエラーが発生する可能性があります。length が 0 の場合には、この計算をスキップする条件を追加することをお勧めします。

src/components/shared/InputSelectMultiple.vue [184-185]

 : (props.options?.length ?? 0)
-focusingListItemIndex.value = (focusingListItemIndex.value + 1) % length
+if (length > 0) {
+  focusingListItemIndex.value = (focusingListItemIndex.value + 1) % length
+}
 
Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies a potential division by zero error when length is 0 and provides a solution to skip the calculation, which is crucial for preventing runtime errors.

9
Enhancement
calcWidth 関数にデフォルトの幅を設定してください。

calcWidth 関数の戻り値が常に props.class の文字列のみを返すように変更されていますが、これにより以前の w-70
が削除されてしまっています。もしデフォルトの幅を設定する意図がある場合は、条件を追加してデフォルト値を設定することをお勧めします。

src/components/shared/InputSelectMultiple.vue [244]

-return `${props.class}`
+return `${props.class} w-70`
 
Suggestion importance[1-10]: 7

Why: The suggestion highlights the removal of a default width (w-70) and recommends reintroducing it, which could be important for maintaining consistent styling. However, it is not as critical as preventing runtime errors.

7

@anko9801 anko9801 requested review from reiroop and removed request for reiroop September 12, 2024 20:45
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.

LGTMです!ありがとうございます
できればレスポンシブ対応したいなーって気持ちはあるのですが、adminページなので使う人も回数もそんなに多くないですしとりあえず今はいいかも?って感じです。

@anko9801 anko9801 merged commit 688d600 into main Sep 24, 2024
7 checks passed
@anko9801 anko9801 deleted the feat/admin_page branch September 24, 2024 13:13
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.

adminページ
2 participants