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

[tds-ui] onChange 함수가 중복 실행되지 않도록 수정합니다. #3532

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

guswl98
Copy link
Contributor

@guswl98 guswl98 commented Jan 3, 2025

PR 설명

onClickonFocus 모두에서 onChange 함수를 실행하고 있어 클릭 이벤트 발생 시 onChange가 두 번 실행되고 있습니다. 이에 onFocus에서만 실행하도록 수정합니다.

하단 논의사항에 대한 코멘트들을 참고하여 onChange 함수를 Tabs 컴포넌트에서 실제 선택된 값이 바뀌었는지 확인 후 실행하도록 수정했습니다.

논의사항

페이지를 벗어났다가 다시 접속해서 새로운 탭을 클릭하는 등의 상황에서도 onFocus가 실행됩니다. 이 때문에 사용자가 탭과 직접 인터렉션을 하지 않아도 onChange가 실행되게 됩니다. 이는 value가 바뀌었을 때 실행되어야 하는 onChange 함수의 의도와는 다른 시점에 실행되고 있습니다. 그러나 Tab의 상위에서 value를 관리하는 로직이 아니기 때문에 현재로서는 Tabs에 넘겨주는 onChange 내에서 구분하는 방법을 사용하는 것이 최선인데요, 더 나은 방법이 있을까요?

@guswl98 guswl98 added this to the 14.0.9 milestone Jan 3, 2025
@guswl98 guswl98 self-assigned this Jan 3, 2025
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 14.25%. Comparing base (5e87a34) to head (13d9a24).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/tds-ui/src/components/tabs/tabs.tsx 0.00% 2 Missing and 1 partial ⚠️
packages/tds-ui/src/components/tabs/tab-base.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3532      +/-   ##
==========================================
- Coverage   14.26%   14.25%   -0.01%     
==========================================
  Files         739      739              
  Lines       10406    10408       +2     
  Branches     3518     3519       +1     
==========================================
  Hits         1484     1484              
- Misses       8495     8496       +1     
- Partials      427      428       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guswl98
Copy link
Contributor Author

guswl98 commented Jan 3, 2025

release-canary

Copy link

github-actions bot commented Jan 3, 2025

v14.0.9-pr-3532.13 has been published!

@guswl98 guswl98 added the tds-ui label Jan 3, 2025
@guswl98 guswl98 requested a review from drakang4 January 3, 2025 07:13
@guswl98 guswl98 marked this pull request as ready for review January 3, 2025 07:13
@guswl98 guswl98 requested a review from a team as a code owner January 3, 2025 07:13
@guswl98 guswl98 requested review from dongoc and gigibean and removed request for a team January 3, 2025 07:13
@dongoc
Copy link
Contributor

dongoc commented Jan 3, 2025

Tabs에서 onFocus를 사용하는 것은 첫 렌더링 시를 의도하는 것이려나요..? 보통은 onClick이 주요 인터렉션이라고 생각해서요!
onFocus를 사용하지 않아도 된다면 onClick만 살려도 되지 않을까 싶은데.. onFocus를 사용하는 예시가 있을까요?👀

@drakang4
Copy link
Contributor

drakang4 commented Jan 3, 2025

onFocus는 마우스 클릭과 키보드 방향키로 탭을 변경하는 경우를 한번에 지원하기 위해서 사용합니다.
더 나은 방법은 onClick이나 onFocus를 사용하지 않고 탭 내부에서 useEffect 써서 index 값이 실제로 변경될 때만 onChange를 실행하는 방법이 될 거 같네요.

@guswl98
Copy link
Contributor Author

guswl98 commented Jan 6, 2025

@drakang4

onFocus는 마우스 클릭과 키보드 방향키로 탭을 변경하는 경우를 한번에 지원하기 위해서 사용합니다.
더 나은 방법은 onClick이나 onFocus를 사용하지 않고 탭 내부에서 useEffect 써서 index 값이 실제로 변경될 때만 onChange를 실행하는 방법이 될 거 같네요.

이건 단일 탭이 아닌 Tabs 컴포넌트에서 관리하는 걸 말씀하시는 걸까요??

@drakang4
Copy link
Contributor

drakang4 commented Jan 6, 2025

네 거기에서 하면 될 거 같아요

@guswl98
Copy link
Contributor Author

guswl98 commented Jan 6, 2025

release-canary

Copy link

github-actions bot commented Jan 6, 2025

v14.0.9-pr-3532.26 has been published!

@guswl98
Copy link
Contributor Author

guswl98 commented Jan 6, 2025

release-canary

Copy link

github-actions bot commented Jan 6, 2025

v14.0.9-pr-3532.27 has been published!

@guswl98
Copy link
Contributor Author

guswl98 commented Jan 6, 2025

@drakang4 13d9a24 에서 적용해보았습니다!

@dongoc 이 주신 질문은 헤론 코멘트 참고하시면 답변이 될 것 같네요.

@dongoc dongoc mentioned this pull request Jan 7, 2025
@guswl98
Copy link
Contributor Author

guswl98 commented Jan 7, 2025

🙇‍♀️

@guswl98 guswl98 merged commit 6cee5a9 into main Jan 7, 2025
15 checks passed
@guswl98 guswl98 deleted the fix/tab-onchange branch January 7, 2025 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants