-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor/#134 kakao to naver #136
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
Conversation
vvan2
left a comment
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.
kakao 에서 naver로 바꾸느라... 고생 많으셨습니다. 이렇게 보니까 리펙하면서 코드 삭제하는게 되게 많겠다 생각이드네요 허허. 리드님 고생하셨고 나도 화이팅 해야겠다.. 어푸띠
| import androidx.compose.ui.unit.dp | ||
| import com.paw.key.core.designsystem.theme.PawKeyTheme | ||
| import com.paw.key.core.util.noRippleClickable | ||
| import com.paw.key.core.extension.noRippleClickable |
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.
extension과 util의 관계를 명확히 해야할 것 같아서용 ㅠㅠ ㅎㅎ;
| title: String, | ||
| onBackClick: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| onClickTitle : () -> 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.
피그마에 컴포넌트가 없어서 잘은 모르겠는데, 선택적으로 콜백 할라고 한건가용
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) { | ||
| permissionLauncher.launch(permissions) | ||
| } | ||
| } No newline at end of file |
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.
👍
|
|
||
| @AndroidEntryPoint | ||
| class MainActivity : ComponentActivity() { | ||
| @SuppressLint("SourceLockedOrientationActivity") |
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.
👀
| data object NavigateUp: RegionSideEffect() | ||
| data object NavigateNext: RegionSideEffect() | ||
| } | ||
| sealed class RegionSideEffect { |
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.
아 나도 contract로 묶어놓은거 있으면 분리해야겠다
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.
좋습니다~
|
|
||
| sealed class UiState<out T> { | ||
| data object Empty : UiState<Nothing>() | ||
| sealed interface UiState<out T> { |
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.
util 쪽 코드를 잘 안짜봐서 모르긴한데, class -> interface로 변경한 이유가 있을까요?(순수 궁금)
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.
우선 UiState의 역할을 저는 그 자체로 동작을 갖지않고 상태만 구분하기 위한 것이라 생각해서 interface로 의도를 명확히 하고 또 class로 사용 시 class 자체의 기본적으로 상속받는 equals, hashcode, toString이런 것들을 자동 상속 받지 않게해서 조금 더 경량화하는 방향을 생각했습니다~!
ISSUE
❗ WORK DESCRIPTION Add commentMore actions
📸 SCREENSHOT
📢 TO REVIEWERS