-
Notifications
You must be signed in to change notification settings - Fork 7
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/#841] 마이페이지 compose 리팩토링 #854
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
첫 PR 축하드립니다!
유빈님 코드 제 스타일이랑 비슷하네요 너무 깔끔해요 편-안
고생하셨습니다!
fun MyPageItem( | ||
text: String, | ||
modifier: Modifier = Modifier, | ||
onButtonClick: () -> Unit = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기본 생성자로 둔 이유가 있나용??
import org.sopt.official.feature.mypage.model.MyPageUiModel | ||
|
||
@Composable | ||
fun MyPageSection(items: List<MyPageUiModel>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ALL
해당 Composable은 항상 unstable 일 것 같은데, 저희 팀 List 사용에 대한 컨벤션이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 List를 사용하지 않고, ImmutableList만 사용하고 있습니다:)
수정해야할 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kotlinx.collections.immutable 라이브러리를 implementation 하셔야합니다!
|
||
import androidx.compose.runtime.Immutable | ||
|
||
sealed interface MyPageUiModel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 저도 헷갈려서 그런데, interface에 @stable 안붙여도 자식클래스들이 Immutable이니까 상관 없으려나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제일 확실한건 metrics 보는거에요. 근데 저라면 그냥 @stable 붙였을듯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 캐스팅 뭘로하냐에 따라 다를듯하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어우 너무 잘하는데요??
근데 Compose로만 이루어진 모듈인데 Activity로 이동하기보단 Navigation으로 이동하는게 좋을거 같긴 합니다. 그건 나중에 전부다 리펙토링이 이루어지고나면 그때 변경해보아요~~~
paddingShape: Dp, | ||
style: TextStyle, | ||
paddingVertical: Dp, | ||
@StringRes text: Int, | ||
onButtonClick: () -> Unit, | ||
modifier: Modifier = Modifier, | ||
isEnabled: Boolean = true, | ||
containerColor: Color = White, | ||
contentColor: Color = Black |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onButtonClick
이라는 이름 보다는 개발자에게 익숙한 onClick
을 하는건 어떨까요? 클릭되는 부분이 2개 이상이라면 이름을 지어주는게 좋지만, 하나만 있다면 익숙한 이름 네이밍이 사용하기 편할거같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 매개변수를 받을 때 "해당값이 없다면 컴포넌트가 목적에 맞는 형태를 가질 수 없는가?"를 기준으로 옵셔널과 필수를 결정해요. 그렇기 때문에 대부분의 값은 저라면 옵셔널로 할 것 같습니다.
그리고 text라는 value를 받아오기 보다 Composable을 받아오는건 어떨까요? 이 컴포넌트를 호출하는 곳에서 Text에 대한 설정을 해준다면 더욱 재사용성이 좋을 것 같아요
.clickable { | ||
onButtonClick() | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.clickable { | |
onButtonClick() | |
}, | |
.clickable ( onClick = onButtonClick ), |
아마 될거에요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 깔끔하고 너무 좋은데요
Scaffold(modifier = Modifier | ||
.background(SoptTheme.colors.background) | ||
.fillMaxSize(), | ||
topBar = { | ||
MyPageTopBar( | ||
title = R.string.toolbar_mypage, | ||
onIconClick = { onBackPressedDispatcher.onBackPressed() } | ||
) | ||
} | ||
) { innerPadding -> | ||
Column( | ||
modifier = Modifier | ||
.fillMaxSize() | ||
.padding(innerPadding) | ||
.background(SoptTheme.colors.background) | ||
.verticalScroll(scrollState) | ||
) { | ||
Spacer(modifier = Modifier.height(20.dp)) | ||
MyPageSection(items = serviceSectionItems) | ||
Spacer(modifier = Modifier.height(16.dp)) | ||
if (isAuthenticated) { | ||
MyPageSection(items = notificationSectionItems) | ||
Spacer(modifier = Modifier.height(16.dp)) | ||
MyPageSection(items = soptampSectionItems) | ||
Spacer(modifier = Modifier.height(16.dp)) | ||
MyPageSection(items = etcSectionItems) | ||
} else { | ||
MyPageSection(items = etcLoginSectionItems) | ||
} | ||
Spacer(modifier = Modifier.height(32.dp)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 Activity에는 Composable을 이용한 화면을 놔두기 보다는 따로 추출하는 편입니다.
Activity에서는 각종 처리들을 하고, Screen과 같은 Composable을 통한 View로서 분리해두는게 가독성이 좋고 나중에 추적하기가 편하다 생각해요
feature/mypage/src/main/java/org/sopt/official/feature/mypage/component/MyPageButton.kt
Outdated
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/component/MyPageButton.kt
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/component/MyPageButton.kt
Outdated
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/component/MyPageButton.kt
Outdated
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageViewModel.kt
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageViewModel.kt
Outdated
Show resolved
Hide resolved
feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageActivity.kt
Outdated
Show resolved
Hide resolved
) : ViewModel() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) : ViewModel() { | |
savedStateHandle: SavedStateHandle | |
) : ViewModel() { | |
private val userActiveState = savedStateHandle.get<UserActiveState>("args")?.userActiveState | |
LaunchedEffect(Unit) { | ||
args?.userActiveState?.let { | ||
viewModel.setUserActiveState(MyPageUiState.User(it)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LaunchedEffect(Unit) { | |
args?.userActiveState?.let { | |
viewModel.setUserActiveState(MyPageUiState.User(it)) | |
} | |
} |
Issue
What is this issue?
Reference
회원
Screen_Recording_20240918_075028_SOPT.DEBUG.mp4
비회원