-
Notifications
You must be signed in to change notification settings - Fork 1
[Koin Project][refactor] 설정 화면 composing UI 부분 #1044
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
base: develop
Are you sure you want to change the base?
Conversation
Add feature setting module
Create setting screen
kongwoojin
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.
고생하셨습니다.
lint 먼저 수정해주세요
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .background(color = backgroundColor) | ||
| .padding(vertical = 13.dp, horizontal = 24.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.
음? 13이요?
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.
figma 는 16 인데
16 으로 하면 기존 설정 화면 대비 매우 커집니다...
일단 수작업으로 조사하면서 가장 알맞은 값을 넣었습니다.
| onNavigationIconClick = onTopbarBackClick | ||
| ) | ||
| }, | ||
| contentWindowInsets = WindowInsets(0, 0, 0, 0) |
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.
insets을 확장했는데, 다른 곳에 insets 처리를 한 코드가 없는 것 같습니다
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.
아 맞다
Add padding insets and navigation bar padding
Feat ktlint
|
chore: Change SettingItem and VersionItem parameter name
feature: Connect SettingViewModel and screen
feature: Add contact click event
Feat ktlint
Change Text in m3 to BasicText
Add google oss licenses lib
Add term page and oss licenses page
Feat ktlint
Delete funtion's unusable lamda
Change URL.kt name to URLConstant.kt
Add Navigation extensions 'goToUrl'
…m/BCSDLab/KOIN_ANDROID into refactor/setting-opensource-marketing
|
Merged #1084 |
Add import Intent
kongwoojin
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.
고생하셨습니다.
코멘트 확인해주세요
| putExtras(bundleOf(*argument)) | ||
| } | ||
|
|
||
| fun Context.goToUrl(uri: Uri, action: String = Intent.ACTION_VIEW, vararg argument: Pair<String, Any?>) { |
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.
그러면 core/Navigator 에서 선언하고 koin/NavigatiorImpl 에 구현하는 방식으로 갈까요?
navigateToContact 가 될 거 같네요. intent 를 생성하는 함수로
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.
Navigator는 내부 화면들끼리 이동하는 함수이니 그냥 ContextUtil 하나 만들어서 처리하면 될 것 같습니다
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.
갑자기 생각난점 확장 함수를 어디다 둬야할까요.
재사용성을 위해 확장함수를 만드는 부분인데 Navigator 은 내부 화면 이동을 위해 넣기 애매합니다.
그냥 feature 에 util 로 넣자니 재사용이 setting 에서만 가능하구요.
그냥 Navigator/Extensions 에 넣을까요? 지금처럼
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.
그냥 core에 넣으면 될 것 같습니다
| ) | ||
| if (showIcon) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_arrow_right), |
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.
ImageVector 사용해주세요
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.
https://developer.android.com/develop/ui/compose/graphics/images/compare?hl=ko
그냥 painter 를 써도 vector 이미지면 자동으로 vectorImage 를 통해 그려 주는 거 같습니다.
밑에 ImageVector.vectorResource 를 사용해서 vectorImage 로 주는 게 있긴 한데
Icon(vectorImage = ) 살짝 뜯어보니 결국 내부에서 다시 Icon(painter =) 호출하는 거 보면 그냥 써도 문제 없을 거 같습니다.
feature/setting/src/main/java/in/koreatech/koin/feature/setting/component/SettingItem.kt
Show resolved
Hide resolved
feature/setting/src/main/java/in/koreatech/koin/feature/setting/component/SettingTitle.kt
Show resolved
Hide resolved
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun SettingScreen( | ||
| viewModel: SettingViewModel = hiltViewModel(), |
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.
여기도요
| Column( | ||
| modifier = modifier | ||
| .fillMaxSize() | ||
| .navigationBarsPadding() |
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의 KoinTopAppBar에서 status bar insets을 처리하고 있어 문제가 되지 않지만, 실제로 SettingScreenImpl에는 navigation bar insets만 적용이 된 상태입니다.
이렇게 되면 KoinTopAppBar가 사라질 경우, status bar와 content가 겹치는 문제가 생길 수 있습니다.
따라서 systemBarsPadding() 사용을 권장합니다.
어차피 상위 컴포넌트에서 status bar insets을 소비했기 때문에 적용되지 않으니깐요
| .fillMaxSize() | ||
| .navigationBarsPadding() | ||
| .padding(contentPadding) | ||
| .consumeWindowInsets(contentPadding) |
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.
navigation bar insets도 소비 처리를 해줘야 할 것 같습니다.
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.
아닌가 생각해보니 navigationBarsPadding()가 소비 처리를 해줬던 것 같네요
확인 후 작업 부탁드려요
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.
expect fun Modifier.navigationBarsPadding(): Modifier 뜯어보니
주석에 | When used, the WindowInsets will be consumed. 이 있네요 소비되는 거 같습니다.
Delete contentPadding parameter
Add notification string resource
Add NorificationSwitchItem component
Add notification menu string resource
…-osslisenses [Koin Project][Refactor] setting 내부 Term(약관) 화면 + ossLicenses(오픈소스) 화면 추가
Add notification itme and switchsubitem components
Add notification screen
Connect setting screen and notification screen
Add component padding
Feat ktlint
Change notification appbar text
Add notification viewmodel
Add login snackbar
…ub.com/BCSDLab/KOIN_ANDROID into refactor/setting-notification
Feat ktlint
…ub.com/BCSDLab/KOIN_ANDROID into refactor/setting-notification
Feat ktlint
…ub.com/BCSDLab/KOIN_ANDROID into refactor/setting-notification
Connect screen and viewmodel
Feat ktlint
…tion-ui [Koin Project][Refactor] 알림 설정 ui 구현 + login snackbar 추가
Delete URLConstant object
Delete Init permitted variable and Add isTypePermitted util
Fix unmatched type
Feat ktlint
kongwoojin
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.
고생하셨습니다.
회의때 이야기 할게 있어서 회의때 이야기 해보고 머지하면 될 것 같습니다
| .clickable { | ||
| onClick() | ||
| } |
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 { | |
| onClick() | |
| } | |
| clickable(onClick = onClick) |
이렇게도 가능할 것 같습니다
Apply fastForEach
…tion [Koin Project][Refactor] setting 에 notification 의 기능적 부분 구현
Remove useless lambda in modifier clickable
Feat ktlint
…-onclick [Koin Project][refactor] Modifier 에 clickable 로 불필요한 lambda 의 제거
PR 개요
이슈 번호: #1042
PR 체크리스트
작업사항
작업사항의 상세한 설명
ui 부분만 만들었습니다.
내부에 많은 세부 화면이 존재하는데
이 부분은 각각 브랜치를 파서 작업하겠습니다.
또한 setting viewModel 과 붙이는 작업도 다른 PR 에 올릴 예정입니다.
논의 사항
version 비교 text 가 어떻게 작동하는질 잘 모르겠네요.
시뮬레이션 환경에서는 보여주고
실제 앱에서는 안보이며 버전에 차이가 날 때에만 표시되는 것 같은데...
아직 제가 원본 코드를 안본 것도 있는데 혹시 아시는 분은 알려주세요.
스크린샷
추가내용