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

#96: Firebase remote config를 추가해요 #106

Merged
merged 10 commits into from
Aug 18, 2024

Conversation

flash159483
Copy link
Contributor

1. 📄 관련된 이슈 및 소개

close #96

2. 🔥 변경된 점

RemoteConfig 추가해요

3. ✅ 필수 체크 사항

  • Firebase Remote Config 라이브러리 추가
  • Splash Screen 추가
  • Firebase Remote Config 연결
  • 데이터 가져오기

4. 📸 작업물 사진 공유(선택)

5. 💡알게된 혹은 궁금한 사항

api에 고정되어 있는 api/v1/를 뺐어요

@flash159483 flash159483 added 🌱기능🌱 새로운 기능을 추가해요 ! 🍩브라우니🍩 24기 정승원 🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. labels Aug 16, 2024
@flash159483 flash159483 self-assigned this Aug 16, 2024
@flash159483 flash159483 requested a review from jeongjaino August 16, 2024 12:48
@flash159483 flash159483 changed the title Feature/flash159483/#96 #96: Firebase remote config를 추가해요 Aug 16, 2024
@jeongjaino
Copy link
Member

스플래쉬 스크린 어떻게 되는지 확인 가능할까영???
보고시퍼잉

@flash159483
Copy link
Contributor Author

스플래쉬 스크린 어떻게 되는지 확인 가능할까영??? 보고시퍼잉

오빠야 아직 아이콘 없어서 설정 못했어.. 기본 안드 splash 뜨고 잠시 흰화면이 나와 그게 지금 splash 나중에 아이콘 나오면 기존 splash 지우고 우리 splash 추가하면 되용

@flash159483 flash159483 reopened this Aug 17, 2024
@flash159483 flash159483 force-pushed the feature/flash159483/#96 branch from 071731c to addd77e Compare August 17, 2024 06:25
Copy link
Member

@jeongjaino jeongjaino 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 +14 to +15
@AndroidEntryPoint
class SplashActivity : ComponentActivity() {
Copy link
Member

Choose a reason for hiding this comment

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

Splash는 따로 모듈로 뺴지 않아도 괜찮을까요 ?

따로 분리해도 괜찮지 않을까요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 app에 있는게 맞다고 생각해서 굳이? 늘어나면 분리하는게 맞죠

Comment on lines +20 to +38
installSplashScreen().apply {
setKeepOnScreenCondition {
return@setKeepOnScreenCondition viewModel.start.value.not()
}
setOnExitAnimationListener { splashScreen ->
ObjectAnimator.ofFloat(
splashScreen.view,
View.TRANSLATION_Y,
0f, splashScreen.view.height.toFloat()
).apply {
interpolator = DecelerateInterpolator()
duration = 500L
val intent = AuthActivity.intent(this@SplashActivity)
startActivity(intent)
doOnEnd {
splashScreen.remove()
finish()
}
start()
Copy link
Member

Choose a reason for hiding this comment

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

이거 애니메이션 적용된 부분은 어떻게 동작하는 거에여 ?? 궁금궁금

Copy link
Member

Choose a reason for hiding this comment

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

installSplashScreen

이 메소드 안드로이드 12부터 적용되는 API인 것 같은데 확인 가능할까요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이거 다음 PR에서 수정했어요

Comment on lines +9 to +16
<entry>
<key>BASE_URL</key>
<value>wespot</value>
</entry>

<entry>
<key>MOCK_BASE_URL</key>
<value>wespot</value>
Copy link
Member

Choose a reason for hiding this comment

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

실제 사용하는 값 디폴트로 넣을 수 있지 않을까요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그래도 문제는 없는데 뭔가 노출시키에는 안전하지는 않은거 같은데 일단 앱 안터지게 막은거에요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이런 정보들은 노출시켜도 문제 없다면 여기에 넣어도 문제 없는데
노출 시키면 장점이 remoteconfig이 오류나서 cancellable되도 앱은 돌아간다는 점이 있죠

Comment on lines +14 to +16
override suspend fun startRemoteConfig(): Boolean {
return suspendCoroutine { continuation ->
remoteConfig.fetchAndActivate().addOnCompleteListener {
Copy link
Member

Choose a reason for hiding this comment

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

suspendCancellableCoroutine을 사용하는게 낫지 않을까영 ?

Copy link
Member

Choose a reason for hiding this comment

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

아래 블로그가 잘 설명해주는 것 같아영
https://tourspace.tistory.com/442

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 cancel되면 안되는 오퍼레이션이라 굳이 cancellable을 쓸 이유는 없다고 생각해요

Comment on lines +17 to +18
continuation.resumeWith(Result.success(true))
}.addOnFailureListener {
Copy link
Member

Choose a reason for hiding this comment

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

함수의 반환 값이 Result가 아닌데, 따로 true만 resume해도 되지 않을까영 ?

Copy link
Member

Choose a reason for hiding this comment

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

false를 반환하는 값이 없어서 Result을 반환 값으로 사용해도 될 것 같구

Boolean 타입을 반환으로 한다면 아래 코드는 어떨까요 ?

return suspendCancellableCoroutine { continuation ->
        val fetchTask = remoteConfig.fetchAndActivate()
        
        fetchTask.addOnCompleteListener { task ->
            if (task.exception != null) {
                continuation.resumeWithException(task.exception!!)
            } else {
                continuation.resume(task.isSuccessful)
            }
        }
        
        fetchTask.addOnFailureListener { exception ->
            Timber.e("fetchRemoteConfig failed: ", exception)
            continuation.resumeWithException(exception)
        }
        
        continuation.invokeOnCancellation {
            fetchTask.cancel()
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저렇게 할 수 있다니 수정했어요

Comment on lines +3 to +7
object RemoteConfigKey {
const val MIN_VERSION = "MIN_VERSION"
const val BASE_URL = "BASE_URL"
const val MOCK_BASE_URL = "MOCK_BASE_URL"
}
Copy link
Member

Choose a reason for hiding this comment

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

util 패키지 보다는 다른 모듈 처럼 config는 어떨까요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그게 더 나아보이네요 수정했어요

Copy link
Member

@jeongjaino jeongjaino left a comment

Choose a reason for hiding this comment

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

고마워 따봉 승원도치야 ~!! 🫶

@flash159483 flash159483 merged commit 5c307dc into develop Aug 18, 2024
24 checks passed
@flash159483 flash159483 deleted the feature/flash159483/#96 branch August 18, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. 🌱기능🌱 새로운 기능을 추가해요 ! 🍩브라우니🍩 24기 정승원
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WS-385 [FEATURE]: Remote Config 추가하기
2 participants