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

[Feature/#908] 오늘의 솝마디 앰플리튜드 & NDS & 에러뷰 적용 #910

Merged
merged 95 commits into from
Oct 16, 2024

Conversation

s9hn
Copy link
Contributor

@s9hn s9hn commented Oct 14, 2024

What is this issue?

Reference

KakaoTalk_Video_2024-10-14-22-00-17.mp4

To Reviewer

  • 흠..... BottomSheet, ErrorDialog, ProgressBar 모두 백그라운드가 Dim되어야하는데 Root스크린의 탑바를 가릴수가 없군요. 마치 손바닥으로 하늘을 가리려 하는 것일까..
  • Error와 Loading시 백그라운드 막아주려고 uiState분기로직이 크게 수정되었습니다. 더 좋은 방법 있으면 조언부탁드립니다.
  • Loading은 따로 정해진게 없는 것 같아서 임시로 넣어놨습니다.
  • 앰플리튜드도 처음해보니까 잘 봐주세요.

s9hn and others added 30 commits September 25, 2024 16:23
- di 모듈 추가
- network api 구현
- mapper 구현
- 레포지토리 인터페이스 생성 및 적용
- 힐트모듈 수정
@s9hn s9hn added the enhancement New feature or request label Oct 14, 2024
@s9hn s9hn self-assigned this Oct 14, 2024
@s9hn s9hn requested a review from a team as a code owner October 14, 2024 12:54
Copy link

height bot commented Oct 14, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@s9hn s9hn changed the title Feature/908 [Feature/#908] 오늘의 솝마디 앰플리튜드 & NDS & 에러뷰 적용 Oct 14, 2024
Comment on lines 59 to 64
val amplitudeTracker = LocalAmplitudeTracker.current.also {
it.track(
type = EventType.VIEW,
name = "view_soptmadi_charmcard",
)
}
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 두면 state가 성공한 경우가 아니어도 무조건 찍히지 않나?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오? 성공 실패는 생각도 못해본 관점이긴한데 그럼 fortuneDetail쪽은 어느쪽에서 해줘야할까요

Copy link
Member

Choose a reason for hiding this comment

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

나라면FortuneAmuletScreen 내부에서 트래커 뷰 찍을듯

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

@l2hyunwoo l2hyunwoo left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.

private var isAnonymous: Boolean = false
private var userId = DEFAULT_ID

init {
refresh()
Copy link
Member

Choose a reason for hiding this comment

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

fetch 정도는 어떠신가요? 처음 데이터 불러올때에도 refresh라고 하나...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fetch는 이제.. 리모트데이터 소스 느낌 ㅋ.. updateUi로..해볼게요?

Copy link
Member

@chattymin chattymin 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 105 to 108
amplitudeTracker.track(
type = CLICK,
name = "uncheck_anonymity",
)
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.

누를 때마다 true false 주기로 했심더

@s9hn s9hn merged commit 520dd11 into develop Oct 16, 2024
1 check passed
@s9hn s9hn deleted the feature/908 branch October 16, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 오늘의 솝마디 앰플리튜드 & NDS & 에러뷰 적용
3 participants