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/request page のデザインを調整 #190

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Conversation

reiroop
Copy link
Contributor

@reiroop reiroop commented Jun 29, 2024

User description

  • フィルターが一時的に消えています
  • 最外部のmarginが調整されています
  • タイトル部分が左寄せになりました
  • 一覧の要素にpaddingが追加されています
  • 一部表記が変更されました
  • アイコンが追加されました
  • レスポンシブ対応をしています

PR Type

enhancement


Description

  • RequestItem.vueにユーザーアイコンを追加し、UIを改善しました。
  • RequestsPage.vueでレスポンシブデザインを改善し、フィルターコンポーネントを削除しました。
  • ページネーションバーの位置を調整し、全体的なレイアウトを改善しました。

Changes walkthrough 📝

Relevant files
Enhancement
RequestItem.vue
RequestItemコンポーネントのUIとレイアウトを改善                                                     

src/components/requests/RequestItem.vue

  • ユーザーアイコンを追加
  • レイアウトの調整(padding, gapの追加)
  • 金額表示のフォントとサイズを調整
+21/-13 
RequestsPage.vue
RequestsPageのデザインとレイアウトを調整                                                             

src/pages/RequestsPage.vue

  • レスポンシブデザインの改善
  • フィルターコンポーネントの削除
  • ページネーションバーの位置調整
+27/-28 

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

@reiroop reiroop requested a review from mehm8128 June 29, 2024 04:17
Copy link
Member

@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.

気になったところ雑にレビューしました
Figmaでauto layoutになってるところは基本flexかgridで作ってpaddingとかgapとかでスペーシングして、できるだけmarginとかはつけないように修正するときれいになりそうです(Figma適当だった頃に昔の僕がmarginつけてたのが悪いけど)

src/components/requests/RequestItem.vue Outdated Show resolved Hide resolved
src/components/requests/RequestItem.vue Outdated Show resolved Hide resolved
src/pages/RequestsPage.vue Show resolved Hide resolved
src/pages/RequestsPage.vue Outdated Show resolved Hide resolved
@mehm8128
Copy link
Member

mehm8128 commented Jul 1, 2024

/review

Copy link

github-actions bot commented Jul 1, 2024

PR Reviewer Guide 🔍

(Review updated until commit b52424e)

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

UI Consistency
新しいユーザーアイコンの追加とレイアウトの変更が行われていますが、アイコンのサイズや配置に関する一貫性が確認されているか再確認が必要です。特に、UserIcon コンポーネントの max-w-7 クラスが適切かどうかを検討してください。

Responsive Design
レスポンシブデザインの調整が行われていますが、特にスマホサイズでの表示に関するTodoが完了しているか確認が必要です。max-w-6xl クラスの使用が適切かどうかも再検討してください。

@mehm8128
Copy link
Member

mehm8128 commented Jul 1, 2024

/improve

@mehm8128
Copy link
Member

マージミスってそうですが大丈夫ですか?
git reset --hard b59aa7dで直るかな

@reiroop
Copy link
Contributor Author

reiroop commented Aug 22, 2024

マージミスってそうですが大丈夫ですか? git reset --hard b59aa7dで直るかな

直せました、ありがとうございます

幅が狭くなった時に縦に並ぶように変更しました。inline-blockを使っています。
@reiroop reiroop requested a review from mehm8128 August 22, 2024 07:16
@mehm8128
Copy link
Member

作業中ならすぐじゃなくても大丈夫ですが、conflictの解消もお願いします🙏

前のコミットの内容からflex-wrapを使う形に修正
画面比率を変えても要素が画面外に出ないようflex-wrapを実装し、alignやgapを調整しました。
StatusChipの左にあったスペースを削除し、右のスペースを20pxに変更しました。
@reiroop reiroop requested a review from mehm8128 August 28, 2024 06:58
RequestsPageのmy-8をp-8に変更して左右に空白を追加しました。
@reiroop
Copy link
Contributor Author

reiroop commented Sep 7, 2024

/review

@reiroop
Copy link
Contributor Author

reiroop commented Sep 7, 2024

/improve

Copy link

github-actions bot commented Sep 7, 2024

PR Code Suggestions ✨

Latest suggestions up to 23fba99

CategorySuggestion                                                                                                                                    Score
Layout issue
flex-grow クラスを削除して、レイアウトの問題を解決します。

RouterLink コンポーネント内で flex-grow クラスが複数の div
要素に適用されていますが、これにより予期しないレイアウトの問題が発生する可能性があります。特に、親要素が flex
ディスプレイを使用している場合、子要素のサイズが不均等になることがあります。これを解決するために、特定の div から flex-grow
を削除することを検討してください。

src/components/requests/RequestItem.vue [36-42]

-<div class="flex flex-grow flex-wrap">
-  <div class="flex-grow">
+<div class="flex flex-wrap">
+  <div>
     <span class="text-xl">{{ request.title }}</span>
     <div class="mt-2">
       <TagsGroup :tags="request.tags" />
     </div>
   </div>
 
Suggestion importance[1-10]: 8

Why: The suggestion to remove flex-grow from certain div elements is valid as it addresses potential layout issues that can arise from uneven sizing of child elements. This is a significant improvement for layout consistency.

8
Design consistency
アイコンのパディング設定を削除して、デザインの一貫性を保ちます。

UserIcon コンポーネントの class 属性に h-5 p-0 を設定していますが、p-0
はパディングをゼロに設定するもので、アイコンの見た目に影響を与える可能性があります。デザインの一貫性を保つために、このパディング設定を削除することを検討してください。

src/components/requests/RequestItem.vue [46]

-<UserIcon class="h-5 p-0" :name="userMap[request.createdBy]" />
+<UserIcon class="h-5" :name="userMap[request.createdBy]" />
 
Suggestion importance[1-10]: 7

Why: Removing the padding setting from the UserIcon component can help maintain design consistency. This suggestion is valid as it addresses a potential design inconsistency, though it is not critical.

7

Previous suggestions

Suggestions
CategorySuggestion                                                                                                                                    Score
Possible bug
SimpleButton の属性を標準的な Vue.js のスタイルに合わせて修正します。

SimpleButton コンポーネントに font-size="lg"padding="md" を指定していますが、これらの属性は Vue.js
の標準的な属性ではありません。カスタムコンポーネントでこれらの属性を適切に処理しているか確認し、必要に応じて修正してください。

src/pages/RequestsPage.vue [68]

-<SimpleButton font-size="lg" padding="md">
+<SimpleButton class="text-lg p-4">
 
Suggestion importance[1-10]: 8

Why: The suggestion identifies a potential bug with non-standard attributes in the SimpleButton component, which could lead to unexpected behavior if not handled properly. This is important for ensuring the component functions as intended.

8
Possible issue
RouterLink のパディングを元の設定に戻します。

RouterLink のクラス属性が flex px-6 py-4
に変更されていますが、この変更により、リンクのパディングが以前よりも大きくなっています。これが意図的なデザイン変更でない場合は、元のパディングに戻すことを検討してください。

src/components/requests/RequestItem.vue [32]

-<RouterLink class="flex px-6 py-4" :to="'/requests/' + request.id">
+<RouterLink class="flex p-2" :to="'/requests/' + request.id">
 
Suggestion importance[1-10]: 7

Why: The suggestion correctly identifies a potential unintended design change. Reverting the padding could be important for maintaining the intended UI layout.

7
Maintainability
レスポンシブデザインを考慮して、不要な最小幅設定を削除します。

新しいページデザインで min-w-160
クラスを使用していますが、これによりコンテンツの最小幅が設定されています。レスポンシブデザインの観点から、この設定が適切であるか確認し、必要に応じて調整してください。

src/pages/RequestsPage.vue [63]

-<div class="min-w-160 flex flex-col">
+<div class="flex flex-col">
 
Suggestion importance[1-10]: 6

Why: The suggestion to remove the min-w-160 class addresses a potential issue with responsive design, which is important for maintainability and user experience on different devices.

6
Enhancement
アイコンのパディングを調整して、UI の一貫性を向上させます。

UserIcon コンポーネントのクラス属性に h-5 p-0 を指定していますが、これはアイコンのサイズとパディングを制御します。デザインの一貫性を保つために、他の UI
要素との間隔を調整することをお勧めします。

src/components/requests/RequestItem.vue [45]

-<UserIcon class="h-5 p-0" :name="userMap[request.createdBy]" />
+<UserIcon class="h-5 p-1" :name="userMap[request.createdBy]" />
 
Suggestion importance[1-10]: 5

Why: The suggestion to adjust padding for UserIcon is minor and subjective, focusing on UI consistency. It does not address a critical issue or bug.

5

Copy link

github-actions bot commented Sep 7, 2024

Persistent review updated to latest commit 23fba99

@reiroop reiroop self-assigned this Sep 28, 2024
@reiroop reiroop marked this pull request as ready for review September 28, 2024 05:41
Copy link

Persistent review updated to latest commit b52424e

@reiroop reiroop linked an issue Sep 28, 2024 that may be closed by this pull request
Copy link
Member

@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.

tailwind移行時に壊れたっぽいところメインですがちょっと気になったところ書きました
あとはそんなに問題なさそうです

src/components/requests/RequestItem.vue Show resolved Hide resolved
<RouterLink class="flex p-2" :to="'/requests/' + request.id">
<div class="mx-2 flex items-center justify-center">
<RouterLink class="flex px-3 sm:px-6 py-4 gap-2 sm:gap-5" :to="'/requests/' + request.id">
<div class="flex items-start justify-center">
<StatusChip :status="request.status" />
Copy link
Member

Choose a reason for hiding this comment

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

status chip

  • 完全な丸になってなさそう
  • Figmaで中身塗りつぶさないやつ使ってるので、そっちの方がいいかもです
    image

src/pages/RequestsPage.vue Outdated Show resolved Hide resolved
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