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

feat!: FormControl,Fieldsetのtitleにアイコンやボタンを含めずマークアップできるよう属性を追加 #5235

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

feat!: FormControl,Fieldsetのtitleにアイコンやボタンを含めずマークアップできるよう属性を追加 #5235

wants to merge 9 commits into from
+162 −91

Conversation

AtsushiM
Copy link
Member

@AtsushiM AtsushiM commented Dec 31, 2024 •
edited
Loading

関連URL

概要

  • FormControl, Fieldsetのtitle属性に設定した値は、 label legend 要素の子としてmarkupされる
  • その際、buttonなど要素の子とするには不適切になりえる場合がある
  • 現状はClusterなどでlayoutした要素をtitleに設定しているので、subActionArea属性を追加したい

変更内容

  • Fieldsetのマークアップに拡張性を持たせるため、legendをVisuallyHidden、UI上のラベルをaria-hiddenにするようにロジックを調整
  • FormControl, FieldsetにsubActionAreaを追加

確認方法

  • マークアップ、属性名などを確認する

@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch 19 times, most recently from 6f55e03 to 9ea6d3d Compare January 1, 2025 01:06
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Jan 1, 2025 •
edited
Loading

Open in Stackblitz

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

commit: 7ef464a

@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch 2 times, most recently from 577e831 to 8533d2d Compare January 1, 2025 01:23
@AtsushiM AtsushiM marked this pull request as ready for review January 2, 2025 04:04
@AtsushiM AtsushiM requested a review from a team as a code owner January 2, 2025 04:04
@AtsushiM AtsushiM requested review from oti and Qs-F and removed request for a team January 2, 2025 04:04
@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch from 520e3b3 to 3bb1b54 Compare January 6, 2025 04:41
@AtsushiM AtsushiM marked this pull request as ready for review January 6, 2025 04:48
@AtsushiM AtsushiM requested a review from uknmr January 6, 2025 04:49
@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch 2 times, most recently from e80e201 to 4ede5a1 Compare January 6, 2025 05:39
uknmr
uknmr requested changes Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@uknmr uknmr left a comment

Choose a reason for hiding this comment

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

全部見きれてないんですが、一旦コメントします!

  • スタイリングは tailwind-variants に寄せたい
  • 早期 return の方が可読性は高そう（不用意なコード差分も出るのでレビューコストも上がってるように感じる）

packages/smarthr-ui/src/components/FormControl/FormControl.tsx Show resolved Hide resolved
packages/smarthr-ui/src/components/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
packages/smarthr-ui/src/components/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
packages/smarthr-ui/src/components/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
@AtsushiM AtsushiM requested a review from uknmr January 8, 2025 00:06
@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch from 4ede5a1 to 675c8ff Compare January 8, 2025 00:30
@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch from 675c8ff to ea182a5 Compare January 8, 2025 00:32
@AtsushiM AtsushiM changed the title feat: FormControl,Fieldsetのtitleにアイコンやボタンを含めずマークアップできるよう属性を追加 feat!: FormControl,Fieldsetのtitleにアイコンやボタンを含めずマークアップできるよう属性を追加 Jan 8, 2025
uknmr
uknmr reviewed Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@uknmr uknmr left a comment

Choose a reason for hiding this comment

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

BREAKING CHANGE する必要ありそうですか?

あと Chromatic の diff 見たかったけど何故か unchanged だった。
image

packages/smarthr-ui/src/components/FormControl/FormControl.tsx Show resolved Hide resolved
packages/smarthr-ui/src/components/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
packages/smarthr-ui/src/components/Fieldset/stories/Fieldset.stories.tsx Outdated Show resolved Hide resolved
@AtsushiM
Copy link
Member Author

AtsushiM commented Jan 8, 2025

BREAKING CHANGE する必要ありそうですか?

#5235 (comment)

↑の状況を発見、調整し、↓のようにhtmlForとlabelIdが不要なpropsになりそうだったため、属性からomitしています

#5235 (comment)

@AtsushiM AtsushiM requested a review from uknmr January 8, 2025 22:35
@yagimushi yagimushi force-pushed the feat-add-props-for-form-control branch from 62bef57 to 38a8849 Compare January 8, 2025 22:58
uknmr
uknmr approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@uknmr uknmr left a comment

Choose a reason for hiding this comment

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

LGTM~

packages/smarthr-ui/src/components/FormControl/FormControl.tsx Outdated Show resolved Hide resolved
packages/smarthr-ui/src/components/FormControl/FormControl.tsx Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uknmr uknmr uknmr approved these changes

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

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

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

Successfully merging this pull request may close these issues.
2 participants
@AtsushiM @uknmr