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

Mainページの作成 #8

Merged
merged 35 commits into from
Mar 10, 2024
Merged

Mainページの作成 #8

merged 35 commits into from
Mar 10, 2024

Conversation

pirotyyy
Copy link
Contributor

PRで実装される機能

  • Mainページの基本的な機能
    • User一覧
    • Hoverしたらその情報がページ右に表示される

今後実装する機能

  • User検索(APIできてからかな)
  • ページング(少し複雑そうだったので、別PRにしようと思います)
  • ナジャさんが作成したキャリアを表示するコンポーネントを結合
  • Responsive

UI

2024-03-10.14.41.02.mov

PRで修正される機能

レビューをお願いしたいポイント

@pirotyyy pirotyyy requested a review from daiki0381 March 10, 2024 05:45
Copy link

Copy link

Copy link
Contributor

@Najah7 Najah7 left a comment

Choose a reason for hiding this comment

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

少々コメント残しました。
全体的にに良かったと思います👍

Copy link
Contributor

Choose a reason for hiding this comment

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

全体的にdivの数が気になるかもです。
コンポーネントの中に隠せるやつは隠した方がいいかも。

ex) <div className="h-max"> <UserSearchForm /> </div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

以下のコミットで対応しました!

Copy link
Contributor

Choose a reason for hiding this comment

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

imho: Tailwindのスタイルだけ変数に入れて、コンポーネントは一行のアローで書くのもアリかも。

>
<div className="flex flex-row items-center">
<div className="mb-2 mr-4">
<UserIcon iconImageHash={user.iconImageHash} size={96} />
Copy link
Contributor

Choose a reason for hiding this comment

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

96は定数として、変数で管理したほうが良さそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらで対応しました!
931e080

import { UserData } from "../../proto/typescript/pb_out/main";

// テストデータ
const users: UserData[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

なかなか量があるので、テストデータは別ファイルに移した方がよさそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらで対応しました!
9ca834c

<div className="w-1/12">
<span
className="material-symbols-outlined"
style={{ fontSize: "96px", color: "#bfdbfe" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

ここだけインラインの理由は?
また、コメントで補足 or 96pxを変数に格納することで意図がわかるようにした方がいいかも。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここで対応しました!
4b8fc1b

}

const CareerPreview = (props: Props) => {
const { user } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

FLY(どっちでもOKです。)

ただの共有っす、こっちの書き方もアリかもです。

const CareerPreview = ({ user }: Props) => {...}

@pirotyyy pirotyyy requested a review from Najah7 March 10, 2024 14:05
Copy link

Copy link
Contributor

@Ishigami100 Ishigami100 left a comment

Choose a reason for hiding this comment

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

LGTM

@pirotyyy pirotyyy merged commit 0230803 into main Mar 10, 2024
1 check passed
@Shion1305 Shion1305 deleted the feature/main-page branch March 10, 2024 14:38
@Najah7 Najah7 linked an issue Mar 13, 2024 that may be closed by this pull request
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.

Main-page pagination
3 participants