Skip to content

Conversation

@sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Apr 12, 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

None

Summary

state가 변할 때마다 AutoFocusHTMLElement.focus() 메서드가 실행되는 버그를 수정합니다.

Details

기존의 ref callback을 사용한 구현을 useState, useLayoutEffect 를 사용한 구현으로 변경했습니다.

Unless you pass the same function reference for the ref callback on every render, the callback will get temporarily detached and re-attached during ever re-render of the component.

리액트 공식 문서를 참고해봤을 때, 함수의 참조가 동일하다면 함수가 매 렌더마다 detach - attach 되면 안되는 거로 예상했지만 실제 동작은 달랐습니다. 상태 변경 시 함수가 계속 실행되어 focus 메서드가 매 렌더마다 실행되었습니다(re-attach).

useStatesetState 함수를 ref callback으로 사용했을 경우엔 DOMNode가 Document에 attach 되었을 때 1번, detach 되었을 때 1번. 즉, 예상한 대로 동작하여 useLayoutEffect 와 함께 사용하는 방식으로 수정했습니다. 플리커링 방지를 위해(브라우저 Paint 시점 이전에 메서드를 실행함을 보장) useEffect 대신 useLayoutEffect 를 사용했습니다.

  • 추가) radix-ui Slot 과 결합되었을 때의 이슈로 추정됩니다.

etc

useMergeRefs : 타입을 교정하고, 함수를 분리하여 가독성을 높였습니다. (참고)

Breaking change or not (Yes/No)

No

References

@sungik-choi sungik-choi added the fix PR related to bug fix label Apr 12, 2023
@sungik-choi sungik-choi self-assigned this Apr 12, 2023
@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

🦋 Changeset detected

Latest commit: e0d953a

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 Minor
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 Apr 12, 2023

Codecov Report

Patch coverage: 93.75% and project coverage change: +0.04 🎉

Comparison is base (a38a63a) 78.11% compared to head (e0d953a) 78.16%.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1    #1279      +/-   ##
===========================================
+ Coverage    78.11%   78.16%   +0.04%     
===========================================
  Files          294      294              
  Lines         3793     3796       +3     
  Branches       841      839       -2     
===========================================
+ Hits          2963     2967       +4     
+ Misses         546      545       -1     
  Partials       284      284              
Impacted Files Coverage Δ
packages/bezier-react/src/hooks/useMergeRefs.ts 92.30% <90.90%> (+10.48%) ⬆️
...ezier-react/src/components/AutoFocus/AutoFocus.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ 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 requested a review from Dogdriip April 12, 2023 03:56
@sungik-choi sungik-choi merged commit dc2e30c into channel-io:next-v1 Apr 12, 2023
@sungik-choi sungik-choi deleted the fix/auto-focus-cb branch April 12, 2023 05:01
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.

1 participant