chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する #5435

AtsushiM · 2025-03-05T06:09:39Z

概要

変更内容

確認方法

pkg-pr-new · 2025-03-05T06:19:09Z

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

commit: 78525a1

AtsushiM · 2025-03-05T23:54:01Z

packages/smarthr-ui/src/components/ErrorScreen/ErrorScreen.tsx

@@ -64,8 +61,8 @@ export const ErrorScreen: FC<Props & ElementProps> = ({
              {links.map((link, index) => (
                <li key={index}>
                  <TextLink
-                    {...(link.target ? { target: link.target } : {})}


Object化して再展開する理由がないため、そのまま設定するようにしました

AtsushiM · 2025-03-05T23:54:58Z

packages/smarthr-ui/src/components/Experimental/SideMenu/SideMenuGroup.tsx

+const GroupTitleText = memo<PropsWithChildren<{ titleElementAs?: ElementType; className: string }>>(
+  ({ titleElementAs: Component = 'p', children, className }) => (
+    <Component>
+      <Text color="TEXT_BLACK" leading="TIGHT" size="S" weight="bold" className={className}>
+        {children}
+      </Text>
+    </Component>
+  ),


一度レンダリングされれば変更が必要になる可能性がかなり低いためmemo化しています

AtsushiM · 2025-03-05T23:56:00Z

packages/smarthr-ui/src/components/FlashMessage/FlashMessage.tsx

-export const FlashMessage: FC<Props & ElementProps> = ({
-  visible,
+export const FlashMessage: FC<ActualProps> = ({ visible, ...rest }) =>
+  visible ? <ActualFlashMessage {...rest} /> : null


visibleがtrueにならない限り、あらゆる判定が不要なため、コンポーネントを切り分け、hookの実行を条件分岐できるようにしました

AtsushiM · 2025-03-05T23:56:45Z

packages/smarthr-ui/src/components/FloatArea/FloatArea.tsx

-type StyleProps = {
-  /** コンポーネントの下端から、包含ブロックの下端までの間隔（基準フォントサイズの相対値または抽象値） */
-  bottom?: CharRelativeSize | AbstractSize
-  /** コンポーネントの `z-index` 値 */
-  zIndex?: number
-}


StylePropsは直後に結合される以外で利用されていないため、分割されている意味が薄い状態でした

imo: 振る舞いに必要な props とスタイリングでしか使わない props で別れてるのは、見方によれば有用だと思います。

@uknmr

imo: 振る舞いに必要な props とスタイリングでしか使わない props で別れてるのは、見方によれば有用だと思います。

まぁそうなんですが、他コンポーネントで同様の理由で別れているものはないためこのコンポーネントで別れていると 再利用されているのか？ などの疑念が生じる可能性もあるし、まとめちゃうほうがいいかなーと思っています。
(もちろん複数回利用されている場合などで分割されているものは良いと思います)

再利用されているのか? と同じように、変更後では「これどこで使われてるの? スタイルだけだっ!」なるので、何を取るかだと思います。
特に方針がないので現状のままで良いのではと思いました。

これどこで使われてるの? スタイルだけだっ!

なるほど？
いや、でもそれ型で制限できない概念ですよね？
(例: StylePropsに書いたからtvに食わせる箇所でしかその属性を利用出来ないわけではない)

特定の値がどこで使われているかを確認できるのは、実際に利用されている箇所を確認するしかないですし、型があることで↑で書かれているような利用がされてしまうのは逆に問題だと思います。

例えば何らかの修正を行う人が対象属性をスタイル以外に利用する際、StlylePropsから移動しないと概念が崩れてしまうことになるし、それらの食い違いが発生する方法を防ぐ仕組みは現在有りません。

これらの問題を事前に防ぐために、むしろまとめるべきだと思います。

修正の正しさを議論したいわけではなく、明確な指針や成果のない修正は入れなくていいのでは? と思ってます。

修正の正しさを議論したいわけではなく、明確な指針や成果のない修正は入れなくていいのでは? と思ってます。

この意見に関して、完全に反対します。
明確な指針、成果は↑で言っている通り

分割されている意味が薄く理由がわからない

分割されていることで発生するデメリットがある（かん違いが発生する可能性があり、かつそれを防げない）

他コンポーネントで同様のことをしている箇所が（少なくとも私が知っている範囲で）ない

です。
この箇所は現時点で 他で採用されていない状態になっており、それによって発生するメリットは実装を見ないと本当かどうか分からないためイマイチ、かつ修正時に見過ごされる可能性がある状態になっており、かつ簡単に修正できる箇所 です。

はっきり言ってしまえば 当たり前のイディオム・指針から外れている状態 だと考えています。
すべて正しい状態にしろ！とは言いませんし、思いませんし、実際不可能ですが、気づいたタイミングで治すべきでは？(型のマージだけでコストほぼかからないですし)

そもそもこの議論の箇所のような型の分割を許容すると至る所で型が分割されかねません。
exportしていたり、複数箇所で参照されるなど、以外で型が分割されていると 型と型をmergeして考える 必要が発生し、開発者の短期記憶に負荷がかかります。

型と型をmergeして考える必要が発生する

どの型に記述するべきか検討する必要が発生する

などです。
これは望むところではないのでは？

またStylePropsの発生タイミングなどを確認しました

370ce97#diff-7390611a4783fb92a84ad63fad0535413ffda21bd090e5b177cb1c1523dad84bR12-R26

この型は FloatArea と過去存在していた内部コンポーネントの Base で同じ部分を共通化するための型という意図だと思われます。
おそらくその後、内部のBaseコンポーネントの削除時、そのまま放置されてきただけです。

改善しようとしているのはわかるんですが、不要なところにも手を入れてレビューする側のコストが上がってるように感じます。
議論コストも高いので、ここで細かな話をする気はないです。
指針を決めるような動き方をしていただければ、何の問題もないです。

指針を決めるような動き方をしていただければ、何の問題もないです。

↑で言っている通り、これは 当たり前のイディオム・指針から外れている状態 だと思っているので、そこの指針を本当に決めますか？
何もかも指針を決めることになりかねないと感じており、個人的にはそこまでやりたくないのですが...

AtsushiM · 2025-03-05T23:58:05Z

packages/smarthr-ui/src/components/FormControl/FormControl.tsx

-        errorListStyle={errorListStyle}
-        errorIconStyle={errorIconStyle}


これらの値はclassNamesにまとめられており、変化する場合は両方同時に変化するため、classNamesをそのまま渡すほうが処理として最適です

packages/smarthr-ui/src/components/Experimental/SideMenu/SideMenuGroup.tsx

uknmr · 2025-03-06T01:43:08Z

packages/smarthr-ui/src/components/FloatArea/FloatArea.tsx

-type StyleProps = {
-  /** コンポーネントの下端から、包含ブロックの下端までの間隔（基準フォントサイズの相対値または抽象値） */
-  bottom?: CharRelativeSize | AbstractSize
-  /** コンポーネントの `z-index` 値 */
-  zIndex?: number
-}


imo: 振る舞いに必要な props とスタイリングでしか使わない props で別れてるのは、見方によれば有用だと思います。

packages/smarthr-ui/src/components/FormControl/FormControl.tsx

…enuGroup.tsx Co-authored-by: KANAMORI Yu <y.kinmori@gmail.com>

…format-class-name-generator-7

AtsushiM · 2025-03-06T22:31:20Z

@uknmr
ちょっと調べた感じ、titleElementAs、listElementA、ともに利用されている箇所がないんですが、この属性って必要なんですっけ？
https://github.com/search?q=org%3Akufu%20SideMenuGroup&type=code

あってもいいと思うんですが、Section > Headingの構造にしたりする場合、ちょっと面倒な気がしており...

AtsushiM · 2025-03-06T22:57:42Z

マークアップから変更してみました
78525a1

uknmr

titleElementAs、listElementA、ともに利用されている箇所がないんですが、この属性って必要なんですっけ

ul を dl として使いたい、p ではなく heading にしたい用途かな、なのであっても良さそうだけど、Heading が使いにくくなるのはわかる。

uknmr · 2025-03-07T07:41:35Z

packages/smarthr-ui/src/components/Experimental/SideMenu/SideMenuGroup.tsx

-        <Text color="TEXT_BLACK" leading="TIGHT" size="S" weight="bold" className={groupTitleStyle}>
+    <li className={classNames.wrapper}>
+      <Section>
+        <GroupHeading titleElementAs={titleElementAs} className={classNames.groupTitle}>


コンポーネントとしてはナビゲーションで、コンテンツ領域に同名の Heading がつくのでマークアップは元のままで良いと思います。

ul を dl として使いたい

型としては ul, ol しか許容していないですね。
あれな話、ul <-> ol しか出来ないなら、使い勝手悪そう...
(そして内部的にliが使われているので、この属性の型としては正しい)

コンポーネントとしてはナビゲーションで、コンテンツ領域に同名の Heading がつくのでマークアップは元のままで良いと思います。

これの意味がイマイチわかっていないのですが、SideMenuGroupの使い方って以下の感じですよね？
Headingはついていないきがします 🤔

<SideMenu> <SideMenuGroup title="個人設定"> <SideMenuItem href="#">アカウント</SideMenuItem> <SideMenuItem href="#" current> 認証設定 </SideMenuItem> </SideMenuGroup> </SideMenu>

あ、これに対応するコンテンツがあり、そちらにHeadingがあるからこっちはHeadingにする必要ないやろ、ってことですかね？

↑だとしたら、ややこしいな...
ざっと調べたんですがSideMenuは基本的にナビゲーションということもあり、Nav 要素でマークアップしてしまえば、内部でHeading使ってもいいんじゃないかなーと思いました。
明確にナビゲーション内の見出しということが伝わるし... (むしろ現状Navじゃないんや)

ぱっと見た感じ、dl > dt, dd 構造にしたくなるけど、SideMenuGroupつかわない場合も考慮するとNav > Section > Heading, ul構造がいいんじゃないかなーと思っています

SideMenuGroupを使わない場合は Nav > ul 構造にできるし。

nav > ul > li > p これが「個人設定」
nav > ul > li > ul > li > p これが「アカウント」
で、構造をあらわせてないですか?

@uknmr さんの言っている構造を表せている、がなんのことかイマイチわかっていませんが俺は

個人設定 や アカウント が何らかの見出しであるべきでは？それにはpよりheadingやdtが良いのでは？

という話をしているのであって、併せて

pとulなどが並んでいてもpはulの見出しにはならないですよね？

という話をしています。
個人設定 や アカウントが見出しである、という認識が揃っているならなぜ pの方が良いと思っているのかが知りたく。

uさんの意見は極論すると 階層構造を持ったSectioningContentはSectioningContentで表す必要がなく、全部pで見出しつけてもいい と言っているように読解してしまうんですが違いますよね？

コンポーネント思考と同じで、この部分だけで切り出して考えるべきな気がしており、 ここは見出し と思われるならそれに適した要素でマークアップするべきでは？というのが俺の意見です。

私はナビゲーションとしての役割の話をしていて、smarthr-ui で言えば

AppHeader

AppNavi

TabBar

Pagination

などのコンポーネントで li の中身が見出しになっている必要を感じたことがないし、慣習的にも見かけてないです。

TabBarとPaginationはそもそもリンク、もしくはボタンに当たる部分しかないので今回の議論から除外していいと思います。

AppNaviに関して言えば機能名に当たる部分はHeadingですが、liに当たる部分にリンク、ボタン以外が存在しないのでこれも議論の対象外でいいと思います。
https://smarthr.design/products/components/app-navi/

AppHeaderはそもそもnavやul, liが使われていないため、参照するのは微妙な気もしますが、例えばドロップダウンを開いた後の 勤怠管理 管理メニュー などは見出しになっています
(AccordionPanelが使われていそうですね)

https://smarthr.design/products/components/app-header/

コンポーネントで li の中身が見出しになっている必要を感じたことがないし、慣習的にも見かけてないです。

私はありますし、ナビゲーション内で見出しをつけること自体は大きなナビゲーションを実装する場合、割とポピュラーでは...?
例えばamazonなどはdivにroleを指定する形ですが明確にheadingを利用しています。
範囲指定などを考慮されていないため、マークアップとしては微妙かもしれませんが...

smarthr内部の例だとトップページに当たる部分では、navが分割されていますが、これも検討の一助にはなりそうです。
(この例の場合、そもそもnav分割しないほうが良さそうな気がします)

AtsushiM added 10 commits March 5, 2025 13:53

chore: classNameGeneratorを定義

5e87254

chore: classNamesを定義

76cba0b

chore: ifをまとめる

53d3abd

chore: actualClassName, actualStyleを定義

faf2b49

chore: classNamesを定義

3874381

chore: ActualFlashMessageを切り出す

5132e69

chore: classNamesを定義

3d7500e

chore: classNamesを定義

37eac80

chore: classNamesを定義

5e31cb5

chore: classNamesを定義

e49f5c9

AtsushiM changed the title ~~chore: Fix format class name generator 7~~ chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する Mar 5, 2025

chore: fix ci

72ae897

yagimushi force-pushed the fix-format-class-name-generator-7 branch from e2c0792 to 72ae897 Compare March 5, 2025 06:18

yagimushi force-pushed the fix-format-class-name-generator-7 branch from db4f2b9 to 72ae897 Compare March 5, 2025 07:08

AtsushiM commented Mar 5, 2025

View reviewed changes

AtsushiM marked this pull request as ready for review March 5, 2025 23:58

AtsushiM requested a review from a team as a code owner March 5, 2025 23:58

AtsushiM requested review from Qs-F and uknmr and removed request for a team March 5, 2025 23:58

uknmr reviewed Mar 6, 2025

View reviewed changes

chore: tvを混ぜる

53cca95

AtsushiM requested a review from uknmr March 6, 2025 03:01

AtsushiM and others added 2 commits March 6, 2025 13:41

Update packages/smarthr-ui/src/components/Experimental/SideMenu/SideM…

4d9737b

…enuGroup.tsx Co-authored-by: KANAMORI Yu <y.kinmori@gmail.com>

chore: fix ci

0b6f406

AtsushiM added 2 commits March 7, 2025 07:22

Merge branch 'master' of https://github.com/kufu/smarthr-ui into fix-…

9d838f6

…format-class-name-generator-7

chore: GroupTitleにrename

de7a1f4

chore: GroupTitleをHeadingに変更し、マークアップを調整

78525a1

yagimushi force-pushed the fix-format-class-name-generator-7 branch from 1e158df to 78525a1 Compare March 6, 2025 22:52

uknmr requested changes Mar 7, 2025

View reviewed changes

AtsushiM requested a review from uknmr March 10, 2025 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する #5435

chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する #5435

AtsushiM commented Mar 5, 2025

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

edited

Loading

AtsushiM Mar 5, 2025

AtsushiM Mar 5, 2025

AtsushiM Mar 5, 2025

AtsushiM Mar 5, 2025

uknmr Mar 6, 2025

AtsushiM Mar 6, 2025 •

edited

Loading

uknmr Mar 6, 2025

AtsushiM Mar 6, 2025 •

edited

Loading

uknmr Mar 7, 2025

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM Mar 9, 2025 •

edited

Loading

uknmr Mar 10, 2025

AtsushiM Mar 10, 2025

AtsushiM Mar 5, 2025

uknmr Mar 6, 2025

AtsushiM commented Mar 6, 2025 •

edited

Loading

AtsushiM commented Mar 6, 2025

uknmr left a comment

uknmr Mar 7, 2025

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM Mar 9, 2025

AtsushiM Mar 9, 2025

AtsushiM Mar 9, 2025 •

edited

Loading

uknmr Mar 10, 2025

AtsushiM Mar 10, 2025 •

edited

Loading

uknmr Mar 11, 2025

AtsushiM Mar 11, 2025 •

edited

Loading

AtsushiM Mar 11, 2025 •

edited

Loading

		errorListStyle={errorListStyle}
		errorIconStyle={errorIconStyle}

chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する #5435

Are you sure you want to change the base?

chore: ErrorScreen, SideMenu, FlashMessage, FloatArea, FormControlの内部ロジックを整理する #5435

Conversation

AtsushiM commented Mar 5, 2025

関連URL

概要

変更内容

確認方法

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

AtsushiM Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM commented Mar 6, 2025 • edited Loading

AtsushiM commented Mar 6, 2025

uknmr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtsushiM Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

AtsushiM Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

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

edited

Loading

AtsushiM Mar 6, 2025 •

edited

Loading

AtsushiM Mar 6, 2025 •

edited

Loading

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM commented Mar 6, 2025 •

edited

Loading

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM Mar 9, 2025 •

edited

Loading

AtsushiM Mar 10, 2025 •

edited

Loading

AtsushiM Mar 11, 2025 •

edited

Loading

AtsushiM Mar 11, 2025 •

edited

Loading