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

作品に関するページの作成 #199

Merged
merged 36 commits into from
Apr 9, 2024
Merged

作品に関するページの作成 #199

merged 36 commits into from
Apr 9, 2024

Conversation

tosaken1116
Copy link
Collaborator

@tosaken1116 tosaken1116 commented Apr 5, 2024

issue番号

closes #6,#7,#17,#19,#20,#24,#25,#26,#27,#77,#79

変更点概要

投稿一覧画面
投稿詳細画面
投稿作成画面の作成

変更内容はUIのみでロジックは別途実装されます

参考文献(あれば)

スクリーンショット(必要であれば)

チェック項目

  • テストが通ったか
  • ビルドが通ったか
  • self assignしたか
  • レビュー優先度のラベルをつけたか

Copy link
Collaborator

@K-Kizuku K-Kizuku left a comment

Choose a reason for hiding this comment

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

ざっくりとは見た
レビュー漏れがあっても許してね

src/components/functional/DropImage/hooks.ts Outdated Show resolved Hide resolved
src/components/functional/DropImage/hooks.ts Outdated Show resolved Hide resolved
return `${Math.floor(diffInSeconds / MONTH)}${DATE_DIFF_LABEL['month']}`;
if (date.getFullYear() === now.getFullYear())
return `${date.getMonth() + 1}/${date.getDate()}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

月/日だけの表示だと,ユーザー視点今年のってわからんくない?
3ヶ月過ぎたらYYYY/MM/DDの表示に統一してもいい気が
(デザイン周りは既に話し合ってそうな気もするので,議論済みならスルーでOK)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

デザイナーと相談しておきます〜


export const Default: Story = {
args: {},
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

storybook上での設定がないのは意図してる?


export const C3_BLOG = 'https://blog.c3js.org/';

export const TOYBOX_DESCRIPTION = 'https://toybox.c3js.org/';
Copy link
Collaborator

Choose a reason for hiding this comment

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

URLの末尾に/があるのとないのがあるのは意図してる?
統一したくない?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

特に意図はないです〜
この辺に関してはまだmock dataなので本実装する時にトレーリングスラッシュ外して実装しておきます

@@ -1,4 +1,4 @@
export const BASE_URL =
process.env.NODE_ENV === 'production'
? process.env['NEXT_PUBLIC_BACKEND_URL'] ?? 'http://localhost:4010'
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
Collaborator Author

Choose a reason for hiding this comment

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

インデックスシグネチャの問題ですね(eslintに設定してある)

envには特定のkey以外はkeyとして存在しているかわからないのでブラケット記法を使うことでkeyをどんなものでも受け取れるようにする感じです

[TypeScript]インデックスシグネチャで感覚的には理解できなかったことまとめ

{work.urls.map((url) => (
<Vertical key={url.url}>
<Link size={20} className="text-orange-pop" />
<Typography>{url.url}</Typography>
Copy link
Collaborator

Choose a reason for hiding this comment

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

このURLのリンクは飛べない仕様であってる?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

現行のtoyboxは飛べる仕様なので修正しておきます〜

Copy link
Collaborator

Choose a reason for hiding this comment

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

image バリデーションエラーは独自で実装しているはずなので,おそらくこれは意図していない挙動だと思われ formに`novalidate`をつければ直りそう(未検証)

@K-Kizuku K-Kizuku self-requested a review April 9, 2024 12:38
Copy link
Collaborator

@K-Kizuku K-Kizuku left a comment

Choose a reason for hiding this comment

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

#199 (comment)
これそのままでいいならOK

@tosaken1116 tosaken1116 merged commit c8039d4 into develop Apr 9, 2024
5 checks passed
@tosaken1116 tosaken1116 deleted the feat/works-ui branch April 9, 2024 12:49
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.

Footerの作成
3 participants