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

Fix wrong typing of TabAction prop #1497

Conversation

yangwooseong
Copy link
Collaborator

@yangwooseong yangwooseong commented Jul 14, 2023

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate changeset for the changes.
  • [Component] I wrote a unit test about the implementation.
  • [Component] I wrote a storybook document about the implementation.
  • [Component] I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox
  • [New Component] I added my username to the correct directory in the CODEOWNERS file.

Related Issue

Fixes #1126

Summary

  • TabActionhref 를 받으면 a 로, 그렇지 않으면 button 으로 동작하지만 event handler 에 대한 타입은 항상 button 에 대한 event handler 로 정의되어 있습니다. 이것을 수정합니다.

Details

  • typescript 에서는 higher order function에 대해 제네릭 타입의 추론이 가능하지만, forwardRef처럼 반환타입이 추가로 속성(displayName, defaultProps)을 가지게 되면 이것이 제대로 동작하지 않습니다. 참고: https://stackoverflow.com/a/58473012/22224797

  • 위에서 제시한 assertion 을 하지 않고 myRef 같이 custom 한 ref prop 을 열거나 글로벌하게 forwardRef 타입을 변경하는 것은 베지어에 적용할 수 없다고 생각하여, assertion 을 해서 아래처럼 타입이 제대로 잡히게 했습니다.

  • As-is

image

[href 있을 때]

image

[href 없을 때]

  • To-be
image

[href 있을 때]

image

[href 없을 때]

Breaking change or not (Yes/No)

  • No

References

@yangwooseong yangwooseong added the fix PR related to bug fix label Jul 14, 2023
@yangwooseong yangwooseong self-assigned this Jul 14, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2023

🦋 Changeset detected

Latest commit: 7e4a5ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@channel.io/bezier-react Patch
bezier-figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (88cb17d) 87.31% compared to head (7e4a5ba) 87.31%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1497   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         280      280           
  Lines        3848     3848           
  Branches      805      805           
=======================================
  Hits         3360     3360           
  Misses        415      415           
  Partials       73       73           
Impacted Files Coverage Δ
...ges/bezier-react/src/components/Tabs/TabAction.tsx 80.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

Chromatic Report

🚀 Congratulations! Your build was successful!

@sungik-choi sungik-choi merged commit b09af7c into channel-io:main Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix PR related to bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance type of TabAction component
2 participants