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

バッジコンポーネントの修正 #335

Closed
wants to merge 5 commits into from
Closed

Conversation

takashi0602
Copy link
Member

@takashi0602 takashi0602 commented Dec 6, 2023

ref: #305

概要

Figmaのデザインが変更されたので、それに合わせてバッジコンポーネントのデザインを修正しました!

その他

PR #334 マージ後にOpenします

@takashi0602 takashi0602 self-assigned this Dec 6, 2023
@uyupunpopunpo
Copy link
Contributor

@takashi0602 takashi0602 marked this pull request as draft December 6, 2023 15:44
@tyokinuhata
Copy link
Member

現状はバッジは技術スタックを表示するためににしか使わないので問題にはならないものの、本来バッジコンポーネントが fastApi とか expo みたいな値を持つのは責任外だよなーとちょっと思った。こういう値はどこか別の場所に定義しても良いかもですね

@takashi0602
Copy link
Member Author

それはそうっすね...
検討します🫡

@takashi0602 takashi0602 marked this pull request as ready for review December 9, 2023 12:39
Comment on lines +20 to +21
Expo: '#000000',
FastApi: '#009485',
Copy link
Member Author

Choose a reason for hiding this comment

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

大文字で始まるのは違和感があるが、良い方法が思い浮かばない🤔
ここだけ大文字でもええか〜という気もあるが、何か良い案とかあります?

↓みたいな配列を定義して、これをkeyに使うとかでも良いんかなと思ったり...🤔

const technologies = ['Expo', 'FastApi'] as const;

Copy link
Member

Choose a reason for hiding this comment

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

Enumっぽく定義するとか🤔

Copy link
Member

Choose a reason for hiding this comment

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

あとは今はこのファイルに置いてても良さそうやけど、このバッジコンポーネント使うページに technologyColors は定義するべきかも?

Copy link
Member

Choose a reason for hiding this comment

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

technologyColors.Expo みたいな感じでアクセスできるなら個人的にはそんなに違和感ないな。コーディング規約的に大文字にするのはOKなんだっけ?

@takashi0602
Copy link
Member Author

レビューしていただいて申し訳ないが、こちらはBCCDに沿って修正するということでCloseします

@takashi0602 takashi0602 closed this Dec 9, 2023
@takashi0602 takashi0602 deleted the fix/305_badge branch December 9, 2023 15:15
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.

3 participants