[Feature/#877] 오늘의 솝마디 콕찌르기 기능 구현 #895
Conversation
- di 모듈 추가 - network api 구현
- mapper 구현 - 레포지토리 인터페이스 생성 및 적용 - 힐트모듈 수정
…into feature/869
feature/fortune/src/main/java/org/sopt/official/feature/fortune/FortuneActivity.kt
Outdated
Show resolved
Hide resolved
feature/fortune/src/main/java/org/sopt/official/feature/fortune/FoundationScreen.kt
Outdated
Show resolved
Hide resolved
feature/fortune/src/main/java/org/sopt/official/feature/fortune/component/FortuneButton.kt
Show resolved
Hide resolved
| url = "https://어쩌구저쩌구/test_fortune_card.png", // 서버에서 받아온 이미지 | ||
| contentDescription = null, | ||
| url = imageUrl, | ||
| contentDescription = "Fortune Amulet", |
There was a problem hiding this comment.
해당 부분 String 추출하는 것이 좋아보입니다! 또한 앱 사용자를 고려하면 한글로 하는 것이 좋을 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
이전에 스트링 추출은 굳이 안해도 될 것 같다는 의견이 있어서 아마 저도 동민이도 따로 추출하지는 않은 것 같습니다.
Description은 사실 주석과도 같아서 한글이든 영어든 상관 없다는 의견이지만, 팀 내에 컨벤션을 만들어두는 것도 좋아보입니다.
There was a problem hiding this comment.
contentDescription은 TTS로 읽어주는 역할로 알고있습니다. 제공할 것이라면 현지화해서 제공하는게 좋지 않을까요??
There was a problem hiding this comment.
저는 굳이 추출할 필요가 없다고 생각해요.
그렇기 때문에 컨벤션을 정하는게 좋은 것 같고, 한글로 하는걸로 정해볼까용?
| init { | ||
| viewModelScope.launch { | ||
| runCatching { | ||
| _state.value = _state.value.copy(isLoading = true) |
There was a problem hiding this comment.
요 부분 위에서 _state를 선언할 때 isLoading 값을 true로 주는 방식도 있을 것 같은데요. 이렇게 하신 이유가 궁금합니다!
There was a problem hiding this comment.
초기값은 isLoading이 false인게 맞다고생각해요!
현재는 해당뷰에서 저 함수가 제일 먼저실행돼서 뷰가 실행되자마자 loading상태가 되지만, 추후에는 아니게 될 수도 있을 것 같아요.
그렇기 때문에 초기값은 false로 하되 서버통신하는 함수를 실행시킬 때 isLoading을 true로 하는게 좋다고 생각합니다.
There was a problem hiding this comment.
그럼 맨 처음 뷰의 state는 어떤 상태인걸까요? 로딩중은 아니지만 데이터가 없는 상황이라고 보면 될까요?
There was a problem hiding this comment.
네넵 맞습니다. 현재는 데이터가 없고, 로딩도 하고있지 않는 상태입니다.
화면이 실행되면 데이터를 받아오는 함수가 실행되는 구조에요
그리고 데이터를 받아오는 함수가 실행될 때 state를 Loading으로 바꿔주는게 이상적인 호출이라고 생각돼요.
기본이 loading이고 데이터를 받아온다. 라기 보다는 데이터를 받아올 때 Loading으로 설정한다. 이게 더 의미상 맞지 않을까 싶어요
| _state.value = _state.value.copy(isLoading = true) | ||
| getTodayFortuneCardUseCase() | ||
| }.onSuccess { todayFortuneCard -> | ||
| _state.update { |
There was a problem hiding this comment.
이 부분만 update 확장함수를 사용하신 이유가 무엇인가요?
There was a problem hiding this comment.
쓰다보니까 저렇게 썼네요.. 별 이유는 없습니당
세훈햄이랑 얘기해보니까 update를 하면 copy를 해서 값을 재할당 하는것과 결론적으로 같더라구요. 그 과정에서 검사하는 과정만 하나 추가되는거고! 그래서 앞으로 저렇게 해도 되겠다... 생각중입니당
There was a problem hiding this comment.
저도 update라는 함수를 처음 봤네요! 근데 이 아티클을 보니 stateFlow에 대한 경쟁 상태가 발생할 수 있기에, update 함수를 사용해주는 것이 좋아 보입니다!
| topStart = 20.dp, | ||
| topEnd = 20.dp, | ||
| ), | ||
| sheetBackgroundColor = Gray800, |
There was a problem hiding this comment.
이 부분은 아직 MDS 적용 전인 걸까요??
There was a problem hiding this comment.
옹.. 이건 리뷰를 이해못했는데, 어떤 MDS가 적용되어야하나요?
There was a problem hiding this comment.
사진처럼 한 번 래핑된? 상태로, 의미가 붙은 채로 존재하는 것도 있더라구요. 저는 대부분 요거 참고해서 사용하는데 앱 팀 피그마에 그냥 Gray800으로 선언되어 있다면 그대로 사용하면 될 것 같습니다!
There was a problem hiding this comment.
아하 이거 다 만들어져있었군요
지금은 피그마에 박혀있는 상태 그대로 가져오긴 했씁니다
| @@ -55,6 +55,9 @@ internal class FortuneDetailViewModel @Inject constructor( | |||
| private val _uiState: MutableStateFlow<FortuneDetailUiState> = MutableStateFlow(Loading) | |||
| val uiState: StateFlow<FortuneDetailUiState> get() = _uiState.asStateFlow() | |||
|
|
|||
| private var isAnonymous: Boolean = false | |||
| private var userId = -1 | |||
There was a problem hiding this comment.
이 -1은 어떤 의미일까요?? 특정 의미를 갖고 있는 거라면 상수로 선언해도 될 것 같아요!
There was a problem hiding this comment.
DB에 ID가 -1이 들어갈일은 없어서 기본값으로 사용했습니다!
의미 있는 상수화가 아니면 안하는 편인데 필요하겠군요 ㅎ.ㅎㅎ
| .padding(horizontal = 20.dp) | ||
| .padding( | ||
| top = 24.dp, | ||
| bottom = 12.dp, | ||
| ), |
There was a problem hiding this comment.
요거 한 함수로 합칠 수 있지 않나요 ??
There was a problem hiding this comment.
horizontal로 생성된 padding 함수는 vertical만 기입가능하고 top bottom 따로 기입 못해줍니당
There was a problem hiding this comment.
제가 생각한건 padding(top = 24.dp, start = 20.dp, bottom = 12.dp, end = 20.dp) 같은 방식이었어요. 사실 큰 문제는 아닌데, 굳이 함수가 두 번 적용되어야 하나? 하는 의문이 있어서 질문 남겼습니다!
There was a problem hiding this comment.
저는 분리하는게 좋은 것 같아요. horizontal로 묶을 수 있는데 굳이 나눈다 라는 생각이 드네요
사람마다 다른듯!
| Box( | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .clip(RoundedCornerShape(12.dp)) | ||
| .background(SoptTheme.colors.primary) | ||
| .clickable(onClick = onClick), | ||
| contentAlignment = Alignment.Center | ||
| ) { | ||
| Text( | ||
| text = title, | ||
| style = SoptTheme.typography.label18SB, | ||
| color = SoptTheme.colors.onPrimary, | ||
| modifier = Modifier.padding(horizontal = 26.dp, vertical = 16.dp) | ||
| ) |
There was a problem hiding this comment.
fillMaxWidth를 사용했음에도 horizontal = 26.dp 만큼의 패딩을 준 이유는 최소한의 크기를 유지하기 위함인가요..?!
There was a problem hiding this comment.
글자가 과도하게 길어졌을때에도 좌우 여백은 필요해서 넣어줬습니다!
만약 저 패딩이 없다면 글자가 엄청 커졌을 때 버튼의 좌우에 딱 붙어있을거에요 :)
| scope.launch { | ||
| selectedIndex = newSelectedIndex | ||
| bottomSheetState.hide() | ||
| viewModel.poke(message) | ||
| } | ||
| }, |
There was a problem hiding this comment.
제가 이 바텀시트의 로직은 잘 모르지만 만약에 바텀시트가 완전히 hide되고 나서 동작을 수행하게 하려면 invokeOnCompletion을 사용해도 좋을 것 같아요!
34기 안팟짱님이 작성해주신 글을 보고 저도 참고했었답니당
There was a problem hiding this comment.
bottomSheetState.hide()는 suspend 함수로써 코루틴 scope내에서만 실행 가능합니다!
ModalBottomSheet와 달리 ModalBottomSheetLayout은 isShowModalBottomSheet과 같은 별도의 Boolean State를 필요로 하지 않습니다.
ModalBottomSheet의 공식문서 구현을 살펴보면 invokeOnCompletion 스코프 내에서 hide, show 중단함수가 완전히 completion되고 나서야 isShow와 같은 Boolean State 값을 변경해주지만, ModalBottomSheetLayout은 변경해줄 state가 없어서 굳이 사용하지 않아도 될 것 같습니다.
…into feature/877
…into feature/877
…into feature/877
What is this issue?
To Reviewer