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

[Bug] 버튼 연타 시 중복 호출 이슈 수정 #243

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

kisa002
Copy link
Contributor

@kisa002 kisa002 commented Aug 30, 2023

Overview (Required)

컴포넌트를 연타할 때 발생하는 중복 호출 이슈를 수정하였습니다.

ISSUE1 뒤로가기 버튼 중복 호출

  • TopAppBar의 뒤로가기(종료) 버튼을 연타하면 Home 화면을 넘어가 뒤로가는 경우가 발생합니다.
  • currentDestination이 Home이 아닌 경우에만 뒤로가도록 수정하였습니다.

ISSUE2 화면 중복 이동

  • 홈 화면에서 session 이동 카드를 TopAppBar 영역에 위치한 후 연타하면 화면이 계속 넘어갑니다.
  • navigate 시 이동할 destination과 currentDestination이 같지 않은 경우에만 이동하도록 함수를 구현하려하였으나
    • contributor 화면에서 AppBar 영역을 터치하여도 투과되어 뒤에 있는 카드가 클릭 되기에
    • TopAppBar에 빈 clickable을 제공하여 클릭 이벤트가 투과하는 것을 방지하였습니다.

Screenshot

ISSUE1 뒤로가기 버튼 중복 호출

Before After

ISSUE2 화면 중복 이동

Before After

- currentDestination이 HomeRoute.route가 아닌 경우에만 popBackStack을 호출하도록 구현
@kisa002 kisa002 changed the title Fix/prevent multiple click [Bug] 버튼 연타 시 중복 호출 이슈 수정 Aug 30, 2023
- 화면 전환 애니메이션이 끝나지 않은 상태에서 카드를 누르면 계속 넘어가기에 TopAppBar에 빈 clickable을 제공하여 중복 호출 방지
@kisa002 kisa002 force-pushed the fix/prevent-multiple-click branch from 18a6184 to 0e41bf9 Compare August 30, 2023 17:06
@github-actions
Copy link

github-actions bot commented Aug 30, 2023

Test Results

18 tests   18 ✔️  6s ⏱️
11 suites    0 💤
11 files      0

Results for commit 7d98e2b.

♻️ This comment has been updated with latest results.

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

이슈 제보와 패치, 상세한 테스트 스크린샷까지 감사합니다 🙇

몇 가지 제안사항이 있으니 확인 부탁드리겠습니다.

Comment on lines +66 to +68
if (!isSameCurrentDestination(HomeRoute.route)) {
popBackStack()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

홈에서도 뒤로가기혹은 네비게이션 동작이 똑같은 상황을 만들어 낼 수 있지 않을까요?
이 부분은 저도 고민해보지 못한 부분이라 비슷한 내용을 담고 있는 이슈 남겨봤습니다.

Copy link
Contributor Author

@kisa002 kisa002 Sep 1, 2023

Choose a reason for hiding this comment

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

근본적으로 방지한다면, Navigate의 경우 PR에 작성해둔 것 처럼
navigate 시 이동할 destination과 currentDestination이 같지 않은 경우에만 이동

하도록 구현한다면 전체 상황에 대해서 방지할 수 있습니다.

뒤로가기의 경우에도 호출되는 모든 popBackStack 부분들에 때 의도하는 destination을 넘겨 해당 destination이 아닌 경우에만 뒤로가도록하는 방식을 채택한다면 마찬가지로 근본적으로 수정가능할 것 으로 보입니다.

다만 전반적으로 하나하나 연타를 해보았을 때 위의 시나리오 외에 중복 호출 되는 곳은 없어 적용하지 않았습니다. 만약 근본적인 차단을 고려하신다면 navigate, popBackStack하는 모든 부분들을 destination을 넘겨 받아 비교 후 처리하도록 구현하도록 하겠습니다. 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

넵, 아직은 확실하지 않은 기능이고 버그에 대한 이슈이므로 이정도로 괜찮을 것 같습니다.
확인 감사합니다

- 외부에서 사용하지 않는 함수 private
- clickable -> pointerInput 대체
@kisa002
Copy link
Contributor Author

kisa002 commented Sep 1, 2023

남겨주신 리뷰 반영하였습니다! 😁

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 👏

@wisemuji wisemuji requested a review from laco-dev September 1, 2023 15:16
@laco-dev laco-dev merged commit 7ad3e27 into droidknights:main Sep 4, 2023
2 checks passed
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.

4 participants