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

feat: AnchorButtonをnext/linkなどのラップされたコンポーネントに対応 #4901

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

masa0527
Copy link
Contributor

@masa0527 masa0527 commented Sep 9, 2024

関連URL

JIRA

概要

TextLinkと同様にnext/link react-routerなどのコンポーネントを使えるようにした。

#4867

変更内容

任意のカスタムコンポーネントをelementAsプロパティを通じて指定できるようにした。

確認方法

sandbox環境などでAnchorButtonを定義して確認

<AnchorButton elementAs={Link} href="/about">
  リンク
</AnchorButton>

Comment on lines 77 to 88
// 型キャストなしで ForwardRefExoticComponent に合わせた型をエクスポートするための処理
type AnchorButtonType = <T extends ElementType = 'a'>(
props: Props<T> & ElementProps<T> & ElementRefProps<T>,
) => ReturnType<FC>

const ForwardedAnchorButton = AnchorButton as unknown as AnchorButtonType & {
displayName: string
}

ForwardedAnchorButton.displayName = 'AnchorButton'

export { ForwardedAnchorButton as AnchorButton }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

正しく型推論が働くようにするTextLinkと同様に型を指定したかったのですが AnchorButtonの場合displayNameが必要だったため、AnchorButtonType にキャストし、さらに displayName を持つようにしています。

もっといい方法あったら指摘お願いします! 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

displayName って普通に React コンポーネントの型に勝手に含まれると思ったんですがそんなことないんですね。
この辺見てもサッパリですが。
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/81f33824fa7f8f0d37e1c899d2be5fbe73f92dce/types/react/index.d.ts#L639-L653

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NamedExoticComponentってのがあったんですね

export const AnchorButton: AnchorButtonType & NamedExoticComponent<Props<any>>
と定義すればいけそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

今のままのがまだわかりやすいか...一旦このままにします!

@masa0527 masa0527 marked this pull request as ready for review September 10, 2024 01:38
@masa0527 masa0527 requested a review from a team as a code owner September 10, 2024 01:38
@masa0527 masa0527 requested review from oti and AtsushiM and removed request for a team September 10, 2024 01:38
@Qs-F Qs-F self-requested a review September 10, 2024 06:42
Copy link

pkg-pr-new bot commented Sep 12, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/kufu/smarthr-ui@4901

commit: fb978dc

Copy link
Member

@AtsushiM AtsushiM left a comment

Choose a reason for hiding this comment

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

👍

const ForwardedAnchorButton = AnchorButton as unknown as AnchorButtonType & {
displayName: string
}

// BottomFixedArea での判定に用いるために displayName を明示的に設定する
Copy link
Contributor

Choose a reason for hiding this comment

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

そもそもこれ本当にまだ必要なんだろうか…?

BottomFixedArea の実装見ても、primary/secondary 属性が Button と AnchorButton のどちらかをランタイムで判定するようなコードには見えないんですがわからない。

/** 表示する `Button` または `AnchorButton` (`variant="primary"` である必要がある) */
primaryButton?: Primary
/** 表示する `Button` または `AnchorButton` (`variant="secondary"` である必要がある)*/
secondaryButton?: Secondary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ほえー、displayName ってそういう使い方してもいいのか…。

Copy link
Contributor

@s-sasaki-0529 s-sasaki-0529 left a comment

Choose a reason for hiding this comment

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

👍 プロダクト側から動作確認できましたー。

@masa0527 masa0527 merged commit a07d257 into master Sep 13, 2024
10 checks passed
@masa0527 masa0527 deleted the feat/anchor-button-add-element-as-props branch September 13, 2024 08:26
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.

4 participants