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

[FEAT/#14] 7주차 과제 구현 #15

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

boiledEgg-s
Copy link
Contributor

Related issue 🛠

Work Description ✏️

  • MVI 적용

Screenshot 📸

  • UI 바뀐건 없습니다!

Uncompleted Tasks 😅

  • 얘도 없어요~

To Reviewers 📢

  • 세미나에서 배운대로 한번 적용해봤습니다~ 반박 다 받으니까 리뷰 많이 남겨주세요!

@boiledEgg-s boiledEgg-s added the enhancement New feature or request label Dec 13, 2024
@boiledEgg-s boiledEgg-s requested a review from a team December 13, 2024 09:02
@boiledEgg-s boiledEgg-s self-assigned this Dec 13, 2024
@boiledEgg-s boiledEgg-s requested review from jm991014, kangyein9892, jihyunniiii and serioushyeon and removed request for a team December 13, 2024 09:02
Copy link

@serioushyeon serioushyeon left a comment

Choose a reason for hiding this comment

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

mvi 적용으로 코드가 원래도 깔끔했는데 더 깔끔해졌네요 ㅋㅋ!! 고생하셨습니다!

imageList = uiState.bannerImgList,
modifier = Modifier.wrapContentHeight()
)
if(uiState.bannerImgList.isNotEmpty()) {

Choose a reason for hiding this comment

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

empty 처리 해주는 거 좋네요!!

interactionState.update { currentState ->
currentState.copy(pressedProgram = program)
private suspend fun deleteProgramFromLocal() {
currentState.pressedProgram?.run {

Choose a reason for hiding this comment

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

가독성 좋네요~

import org.sopt.and.core.viewmodel.UiEvent
import org.sopt.and.domain.entity.Program

sealed class MyPageUiEvent: UiEvent {

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

@kangyein9892 kangyein9892 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 +25 to +31
setState {
copy(
bannerImgList = recommendationRepository.getBannerImages(),
recommendations = recommendationRepository.getRecommendations(),
rankedSeries = recommendationRepository.getMostPopularSeries()
)
}
Copy link

@kangyein9892 kangyein9892 Dec 18, 2024

Choose a reason for hiding this comment

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

그 세미나할때 팟장지현이가

✔️ handleEvent
아까 잠깐 설명했는데! 발생하는 Event에 따라 State를 변경시켜주는 친구입니다.
여기서 주의할 점! **setState 함수를 호출하는 것은 여기에서만 이루어져야 합니다.**

라고 했는데 이건 상관이 없을까요...?! 저도 잘 몰라서 물어봅니다...!

Copy link
Contributor

Choose a reason for hiding this comment

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

상태를 변경하는 로직을 한 군데에서만 관리하기 위해서 setState에서만 상태 변경을 해주시는 것이 좋습니다! (자세한 내용은 세미나 자료 참고,,)

Comment on lines +76 to +81
suspend fun getMyHobby(token: String) {
myHobbyRepository.getMyHobby(token)
.onSuccess { hobby ->
setState { copy(hobby = hobby.hobby) }
}
}
Copy link

@kangyein9892 kangyein9892 Dec 18, 2024

Choose a reason for hiding this comment

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

이것도 위의 코리랑 비슷한 맥락인 것 같네요...!! 저도 이부분을 고민하다가 해결 못하고 일단 이 방법으로 진행하기는 했는데 다른 방법이 있을까요...? 같이 고민해보면 좋을 것 가탕요...!! ㅠㅠ (근데 종명오빠 코드 보면 이벤트로 발생시켜주고 처리한것 같은데 맞나요...?)

Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 이벤트 발생 시켜주고 넘겨주는 인자 값으로 handleEvent에서 state 값을 업데이트 시켜주면 될 것 같아요 ~

if (uiState.value.starredProgram.contains(program)) return@launch
private suspend fun insertProgramToLocal(program: Program) {
if (currentStarredState.contains(program))
return

Choose a reason for hiding this comment

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

코드 이해가 부족해서 그런 걸수도 있지만 여기서 return이 왜 필요한가요?!

Choose a reason for hiding this comment

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

저두 궁금합니다!!

Comment on lines +31 to +36
setState {
copy(
popularSeries = popularProgramRepository.getPopularSeries(),
popularMovies = popularProgramRepository.getPopularMovies()
)
}

Choose a reason for hiding this comment

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

요기도....!

Copy link

@jm991014 jm991014 left a comment

Choose a reason for hiding this comment

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

리뷰 달 게 있나 하면서 쭉 내려보는데 벌써 끝까지 내려왔네요.. 깔끔한 코드 7주간 많이 배웠습니다ㅎㅎ 고생하셨어요!!!!

Comment on lines +95 to +106
onIdChange = { newValue ->
viewModel.setEvent(SignInUiEvent.OnIdTextFieldChanged(newValue))
},
onPasswordChange = { newValue ->
viewModel.setEvent(SignInUiEvent.OnPasswordTextFieldChanged(newValue))
},
onLoginClick = {
viewModel.setEvent(SignInUiEvent.OnSignInButtonClicked)
},
onSignUpClick = {
viewModel.setEvent(SignInUiEvent.OnSignUpButtonClicked)
},

Choose a reason for hiding this comment

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

가독성 좋네요~

if (uiState.value.starredProgram.contains(program)) return@launch
private suspend fun insertProgramToLocal(program: Program) {
if (currentStarredState.contains(program))
return

Choose a reason for hiding this comment

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

저두 궁금합니다!!

Copy link
Contributor

@jihyunniiii jihyunniiii 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 +25 to +31
setState {
copy(
bannerImgList = recommendationRepository.getBannerImages(),
recommendations = recommendationRepository.getRecommendations(),
rankedSeries = recommendationRepository.getMostPopularSeries()
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

상태를 변경하는 로직을 한 군데에서만 관리하기 위해서 setState에서만 상태 변경을 해주시는 것이 좋습니다! (자세한 내용은 세미나 자료 참고,,)

Comment on lines +76 to +81
suspend fun getMyHobby(token: String) {
myHobbyRepository.getMyHobby(token)
.onSuccess { hobby ->
setState { copy(hobby = hobby.hobby) }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 이벤트 발생 시켜주고 넘겨주는 인자 값으로 handleEvent에서 state 값을 업데이트 시켜주면 될 것 같아요 ~

Comment on lines +39 to +41
is MyPageUiEvent.OnLogoutButtonClick -> {
setSideEffect(MyPageSideEffect.OnLogout)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

요거는 상태를 변경시켜주는 로직이 아닌 것 같은데 handleEvent에서 처리해주는 이유가 있나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 7주차 과제
5 participants