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: TextLinkをnext/linkなどのラップされたコンポーネントに対応 #4867

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

masa0527
Copy link
Contributor

@masa0527 masa0527 commented Aug 23, 2024

Related URL

https://smarthr.atlassian.net/browse/SHRUI-1018

Overview

UpwardLink をSPA対応するために、内部で使われている TextLinkコンポーネントにelementAsプロパティを追加しました。このelementAsプロパティに要素を指定した場合、指定された要素に固有のpropsが自動的に引き継がれるようになります。

What I did

  • TextLinkコンポーネントにジェネリック型Tを導入しました。
  • この変更は、既存のTextLinkコンポーネントの使用方法に影響を与えません。
  • elementAsと定義したのはnext/linkがasというpropsを持っていたので被りが発生しないようにしています。

next/linkを利用した場合

import Link from 'next/link'
import { TextLink, UpwardLink } from 'smarthr-ui'

<TextLink elementAs={Link} href="/about" suffix={<FaArrowRightIcon />}>
  smarthr-ui with next/link
</TextLink>

<UpwardLink elementAs={Link} href="/" indent={false}>
  前へ戻る
</UpwardLink>

Capture

nextのSandbox環境での例
TextLinkのStyleを適用しつつ next/linkが問題なく動作する
image

@masa0527 masa0527 marked this pull request as ready for review August 26, 2024 02:35
@masa0527 masa0527 requested a review from a team as a code owner August 26, 2024 02:35
@masa0527 masa0527 requested review from moshisora and atzzCokeK and removed request for a team August 26, 2024 02:35
</li>
</ol>
</main>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

sandbox どう扱うか迷いますね。
今回はこうしないと確認は出来ないけど、かといってここに実装してるからって何かが担保できるわけでもないですし。 (sandbox については型チェックすらしてない)

方針決まるまでは特に何も実装しないでサンドボックスとして使えるまっさらな状態で良いかもしれません。
(個人的にはサーバーコンポーネント対応の中でこのサンドボックスを有効活用したい)

Copy link
Contributor

Choose a reason for hiding this comment

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

sandboxに置かないなら

type Props = Omit<ComponentPropsWithoutRef<'a'> & { to: ComponentPropsWithoutRef<'a'>['href'] }

const Link = (props: Props) => <a {...props} href={to} />

的なのをtestにおいてもいいかも、と思いましたがsandboxでも良さそうでした!

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.

👍 next/link と react-router/link 両方で動作確認できました!型エラーもリンターエラーも発生しないし最高です!

Copy link
Contributor

@Qs-F Qs-F left a comment

Choose a reason for hiding this comment

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

よさそうでした!!

一応参照元を見えるところに残しておきます:
参考にした実装

</li>
</ol>
</main>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

sandboxに置かないなら

type Props = Omit<ComponentPropsWithoutRef<'a'> & { to: ComponentPropsWithoutRef<'a'>['href'] }

const Link = (props: Props) => <a {...props} href={to} />

的なのをtestにおいてもいいかも、と思いましたがsandboxでも良さそうでした!

@Qs-F Qs-F merged commit 68cf805 into master Aug 27, 2024
8 checks passed
@Qs-F Qs-F deleted the textlink-add-element-as-props branch August 27, 2024 05:10
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