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

[Contributor] 컨트리뷰터 카드 클릭 시 Github URL 이동 #231

Merged
merged 6 commits into from
Aug 23, 2023

Conversation

JaesungLeee
Copy link
Contributor

Issue

Overview (Required)

  • KngithsCard 클릭 시 LocalUriHandler를 통해 깃허브 페이지 이동

Links

  • none

Screenshot

Before After

@github-actions
Copy link

github-actions bot commented Aug 21, 2023

Test Results

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

Results for commit f45aafe.

♻️ This comment has been updated with latest results.

@@ -197,7 +199,10 @@ private fun ContributorItem(
lightId = R.drawable.ic_contributor_placeholder_lightmode,
darkId = R.drawable.ic_contributor_placeholder_darkmode,
)
KnightsCard(modifier = modifier) {
KnightsCard(
onClick = { uriHandler.openUri(contributor?.githubUrl.orEmpty()) },
Copy link
Contributor

Choose a reason for hiding this comment

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

uriHandler.openuri("") 가 호출되면 예외가 발생하며 앱이 종료됩니다.

안전하게 호출될 수 있도록 해보면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intent data로 빈 문자열이 전달될 경우 발생하는 예외에 대해 확인했습니다.
이 경우 2가지 대안을 고려중인데 어떤게 기획에 더 부합한지 의견 구하고 싶습니다.

  1. droidknights 페이지로 이동
  2. 클릭 이벤트 무시

감사합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JaesungLeee
후원사에 대한 기능이기 때문에 이벤트를 무시하는 방향이 좋을 것 같습니다.
*클릭했더니 홈페이지로 이동하는것으로 기대하기 어려움

이 경우 클릭 enabled 처리를 같이 확인 해 주시면 됩니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다. 612f249

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.

고생 많으셨습니다! 👍

KnightsCard(modifier = modifier) {

KnightsCard(
enabled = contributor?.githubUrl?.isNotEmpty() ?: false,
Copy link
Member

Choose a reason for hiding this comment

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

이런 로직은 별도의 UI 모델을 만들어 관리하는것도 테스트하기 편할 것 같아요. 지금 상태로도 괜찮으니 꼭 반영하진 않으셔도 됩니다 🙏

Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

확인했습니다.
감사합니다.

@laco-dev laco-dev merged commit a1a2e46 into droidknights:main Aug 23, 2023
@JaesungLeee JaesungLeee deleted the feature/#224 branch August 23, 2023 13:58
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