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

提出一覧テーブルと汎用テーブルの実装 #87

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

Conversation

TwoSquirrels
Copy link
Member

@TwoSquirrels TwoSquirrels commented Dec 26, 2024

close #67

GitHub Copilot による要約

このプルリクエストには、アプリケーション内のさまざまなコンポーネントの機能とレイアウトを強化するための変更が含まれています。主な変更点は、モック API レスポンスの更新、新しいリスト表示テーブルおよびページ切り替えコンポーネントの追加、ユーティリティ関数の実装、およびビューの改善です。

モック API レスポンス:

  • src/api/mock/front-back-api-mock.json 内のモック API レスポンスにおいて、languageId フィールドの修正を行い、適切な JSON フォーマットを確保しました。
    [1]
    [2]

新しいコンポーネント:

  • ListingTable.vue コンポーネントを追加し、カスタマイズ可能な列と行を持つ表形式のデータを表示できるようにしました。
  • PageSwitcher.vue コンポーネントを追加し、前後のページ遷移ロジックを含むページネーションを実装しました。

ユーティリティ関数:

  • src/utils/date.tsdateToString 関数を導入し、特定のロケールで日付をフォーマットできるようにしました。

ビューの改善:

  • UserView.vue を更新し、サイドナビゲーションを追加するとともに、ルートパラメータに基づいてユーザーデータを取得するようにしました。
  • UserSubmissions.vue を強化し、ユーザーの投稿を表形式で表示し、ページネーションとデータ取得のロジックを組み込みました。

補足

  • 汎用的な ListingTable コンポーネントを作り、それを使用する感じで実装しました。サイト内の他のテーブルも基本的にこれを使うことを想定しています。
    セルの中身の指定に slot を用いることで、文字列に限らず、リンク、アイコン、コンポーネントなどを自由に指定できるようになっています。
  • Mock の JSON が壊れていたのを修正しましたが、Actions の方に適用されてないのでなんとかしないといけません。
  • Mock が細かなパラメータに対応できていないため、ページネーションなどの挙動が確認しにくくなっています。
    現状だと totallimit と等しくなってしまうため、loadSubmissions 関数の const summaries: SubmissionSummaries = await response.value() のすぐ下に summaries.total = 3141 などを書き加えて複数ページを試すことができます。
  • なお、ページ遷移ボタンのデザインが Figma に無かったため、とりあえず AtCoder のような感じにしています。
  • TODO コメントにもある通り、スマホなど幅の狭いデバイスに対応できていません。(レイアウトが崩れます)
  • TODO コメントにある通り、テーブルのフィルターやソートの機能はまだ実装されていません。この機能を実装するときは、ListingTable にその機能を UI 含め実装し、ListingTable を使う側がその新機能に対応するよう修正するという形を想定しています。

@TwoSquirrels TwoSquirrels marked this pull request as ready for review February 19, 2025 23:04
@TwoSquirrels TwoSquirrels changed the title Submissions table 提出一覧テーブルと汎用テーブルの実装 Feb 19, 2025
@mathsuky mathsuky self-requested a review February 20, 2025 08:09
Copy link
Collaborator

@mathsuky mathsuky left a comment

Choose a reason for hiding this comment

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

実装ありがとうございます!再利用性が高いコードを作ってくださっていただけたので大変助かります!
いくつかコメントをしたので確認いただければとおもいます。それから,API周りでエラーが出ていたので,その修正をmainに取り込みました。マージしてから作業を続けていただければと思います。
Mockの方の指摘もありがとうございます!別PRでまとめて直そうかなー?

// TODO: Fetch user data
</script>

<!-- TODO: Fix the layout on mobile -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

mobileのデザインは一旦考慮しないでおいて,このPRでは作業しなくて良いです。

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO コメント消しときました

Comment on lines 36 to 42
const response = await new SubmissionsApi().getSubmissionsRaw({
orderBy: 'submittedAtDesc',
username,
limit: rowPerPage,
offset: page.value * rowPerPage
})
const summaries: SubmissionSummaries = await response.value()
Copy link
Collaborator

Choose a reason for hiding this comment

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

getSubmissionsを用いればまとめられると思います。

Copy link
Member Author

Choose a reason for hiding this comment

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

多分もともとレスポンスコード取るために Raw 使ってたんですが、全部 try-catch に丸投げすることに変えて意味が無くなってたので普通のに戻しました

})
const summaries: SubmissionSummaries = await response.value()
totalSubmissions.value = summaries.total!
Copy link
Collaborator

Choose a reason for hiding this comment

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

非nullアサーションするよりnullやundefindedであった時のデフォルト値を明示的にセットする方が良さそう。他の部分も同様です。

Copy link
Member Author

Choose a reason for hiding this comment

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

Non-null Assertion をなくしました。それに伴い、Nullish になり得ないけど Non-null Assertion を避けたところには補足を書き残しておきました

<!-- Nullish になり得ない所でも型安全性のため Non-null Assertion はしない -->

}>()
const emit = defineEmits<{ switch: [page: number] }>()
// previous pages (begin, ..., -32, -16, -8, -4, -3, -2, -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

日本語話者がメインでメンテすることが想定されるのでコメントは日本語でもいいかなー?と思いますが,これは好みの問題なので何方でも良いです!

Copy link
Member Author

Choose a reason for hiding this comment

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

ここは英語のままにしましたが、複雑な箇所や意図を伝えたい箇所では日本語でコメントを書きました

<div>
<h1>User</h1>
<RouterView />
<div class="flex gap-[max(2rem,calc(25dvw-16rem))] px-[max(2rem,calc(25dvw-16rem))]">
Copy link
Collaborator

Choose a reason for hiding this comment

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

今はデザインが確定していないので全く問題ないですが,将来的にはこのようなArbitrary valueは削除することになることだけよろしくお願いします:man_bowing:

Copy link
Member Author

Choose a reason for hiding this comment

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

こういう細かな調整は <style> でやる感じですかね?
仮デザインなのでとりあえずいじらずそのままにしてます

デザインはプロジェクト全体を通して仮であるため、ここだけ TODO コメントで明記する必要はない
Nullish になり得ない所も型安全性のために避ける
非本質な URL クエリは切り分けるべきなので
提出一覧はユーザーページ以外でも使うことが想定されるため
@TwoSquirrels
Copy link
Member Author

追加で気になったところのリファクタリングもしましたが、修正版できたはずです

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

Successfully merging this pull request may close these issues.

提出一覧テーブル
2 participants