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.

Sign up for GitHub

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

Jump to bottom

chore: Heading/LanguageSwitcher, Icon, InformationPanelの内部ロジックを整理する #5433

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

chore: Heading/LanguageSwitcher, Icon, InformationPanelの内部ロジックを整理する #5433

wants to merge 15 commits into from
+117 −124

Conversation

AtsushiM
Copy link
Member

@AtsushiM AtsushiM commented Mar 5, 2025

関連URL

概要

変更内容

確認方法

@AtsushiM AtsushiM changed the title chore: Fix format class name generator 5 chore: Heading/LanguageSwitcher, Icon, InformationPanelの内部ロジックを整理する Mar 5, 2025
AtsushiM
AtsushiM commented Mar 5, 2025
View reviewed changes
packages/smarthr-ui/src/components/Header/LanguageSwitcher/LanguageSwitcher.tsx Outdated
const ARROW_KEY_REGEX = /^Arrow(Up|Down|Left|Right)$/
const ARROW_UPS_REGEX = /^Arrow(Up|Left)$/

const appLauncher = tv({
const ON_KEY_DOWN_CONTENT = (e: KeyboardEvent<HTMLDivElement>) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

stateなどと依存関係がないため、定数化しました

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Mar 5, 2025 •
edited
Loading

Open in Stackblitz

npm i https://pkg.pr.new/kufu/smarthr-ui@5433

commit: e240c52

@AtsushiM AtsushiM marked this pull request as ready for review March 5, 2025 04:41
@AtsushiM AtsushiM requested a review from a team as a code owner March 5, 2025 04:41
@AtsushiM AtsushiM requested review from oti and yuzuru-akiyama and removed request for a team March 5, 2025 04:41
@uknmr uknmr requested review from a team and yt-ymmt and removed request for yuzuru-akiyama and a team March 5, 2025 22:23
uknmr
uknmr reviewed Mar 5, 2025
View reviewed changes
packages/smarthr-ui/src/components/Header/LanguageSwitcher/LanguageSwitcher.tsx Outdated
const ARROW_KEY_REGEX = /^Arrow(Up|Down|Left|Right)$/
const ARROW_UPS_REGEX = /^Arrow(Up|Left)$/

const appLauncher = tv({
const ON_KEY_DOWN_CONTENT = (e: KeyboardEvent<HTMLDivElement>) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo: 関数も大文字にするの見慣れない感じがしました。特に定数感を強めなくても handleKeyDownContent や keyDownContentHandler などでも十分に見えます。

Copy link
Member Author

Choose a reason for hiding this comment

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

あー、どうなんだろう。
一応定数であることを明示するメリットはあるかなーと思いますが...

Copy link
Collaborator

Choose a reason for hiding this comment

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

（個人的には）あまり見慣れないなぁと思いました。
JS でファイルグローバルにある関数はみんな定数としての関数なのか? という感じ。

Copy link
Member Author

Choose a reason for hiding this comment

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

難しいな。
でも関数が定数形式の名称になってると、実行するときとかに違和感あるのは 🦀 なので キャメルケースに変更します。
onかhandleかはどうしましょうね....
#5433 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

キャメルケースに戻すのは自分に良さそうに思いました！
on or handleですが、Reactに合わせるならhandle が良いかもしれませんね〜！

https://ja.react.dev/learn/responding-to-events#:~:text=%E6%85%A3%E7%BF%92%E7%9A%84%E3%81%AB%E3%80%81%E3%82%A4%E3%83%99%E3%83%B3%E3%83%88%E3%83%8F%E3%83%B3%E3%83%89%E3%83%A9%E3%81%AF%20handle%20%E3%81%AE%E5%BE%8C%E3%82%8D%E3%81%AB%E3%82%A4%E3%83%99%E3%83%B3%E3%83%88%E3%81%AE%E5%90%8D%E7%A7%B0%E3%82%92%E3%81%A4%E3%81%AA%E3%81%92%E3%81%9F%E5%90%8D%E5%89%8D%E3%81%AB%E3%81%99%E3%82%8B%E3%81%93%E3%81%A8%E3%81%8C%E4%B8%80%E8%88%AC%E7%9A%84%E3%81%A7%E3%81%99%E3%80%82onClick%3D%7BhandleClick%7D%E3%80%81onMouseEnter%3D%7BhandleMouseEnter%7D%20%E3%81%AA%E3%81%A9%E3%81%8C%E3%82%88%E3%81%8F%E8%A6%8B%E3%82%89%E3%82%8C%E3%81%BE%E3%81%99%E3%80%82

Copy link
Member Author

Choose a reason for hiding this comment

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

onになりました。

https://kufuinc.slack.com/archives/CGC58MW01/p1741237708852539

まとめ

  • どちらかに明確なメリットはないため、厳格に決定する必要はなさそう
  • その上で、smarthr-uiはコンポーネントライブラリなので既に onXxxx系の属性が存在する
  • 最終的にonXxxx系属性に設定されることになるものはonXxxxという名前にしたほうがわかりやすい
    • 逆にonXxxx系属性に設定されない関数に関してはこのルールを適用することはない（しない方が良い場合が多い）
  • 上記から記述的に短いこともありonXxxxというフォーマットを採用します

@AtsushiM AtsushiM requested a review from uknmr March 6, 2025 03:03
yt-ymmt
yt-ymmt reviewed Mar 6, 2025
View reviewed changes
packages/smarthr-ui/src/components/Header/LanguageSwitcher/LanguageSwitcher.tsx Outdated
@@ -150,7 +153,7 @@ export const LanguageSwitcher: FC<Props & ElementProps> = ({
className={classNames.switchButton}
label={decoratedTexts.triggerLabel}
/>
<DropdownContent onKeyDown={ON_KEY_DOWN_CONTENT} role="presentation">
<DropdownContent role="presentation" onKeyDown={ON_KEY_DOWN_CONTENT}>
Copy link
Contributor

Choose a reason for hiding this comment

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

ここも同様の理由でキャメルケースに戻すと良いですかね？

yt-ymmt
yt-ymmt reviewed Mar 6, 2025
View reviewed changes
Copy link
Contributor

@yt-ymmt yt-ymmt left a comment

Choose a reason for hiding this comment

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

細かいところですがコメントしました！

@AtsushiM AtsushiM requested a review from yt-ymmt March 6, 2025 23:05
yt-ymmt
yt-ymmt approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@yt-ymmt yt-ymmt left a comment

Choose a reason for hiding this comment

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

yay!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yt-ymmt yt-ymmt yt-ymmt approved these changes

@oti oti Awaiting requested review from oti oti is a code owner automatically assigned from kufu/group-dev-smarthr-ui-reviewers

@uknmr uknmr Awaiting requested review from uknmr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AtsushiM @yt-ymmt @uknmr