-
Notifications
You must be signed in to change notification settings - Fork 1
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/mz 149 navigation bar #15
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.
navigation 패키지를 component 하위 패키지로 옮겨주세요!
feature/navigator에 있을만한 내용이 desginsystem에 섞여 있어서 관련해서 리뷰드렸어요!
enum class NavigationItem( | ||
@DrawableRes val selectedIcon: Int, | ||
@DrawableRes val unselectedIcon: Int, | ||
val tabName: String, | ||
) { | ||
SENT( | ||
selectedIcon = R.drawable.ic_sent_filled, | ||
unselectedIcon = R.drawable.ic_sent_outlined, | ||
tabName = "보내요", | ||
), | ||
RECEIVED( | ||
selectedIcon = R.drawable.ic_received_filled, | ||
unselectedIcon = R.drawable.ic_received_outlined, | ||
tabName = "받아요", | ||
), | ||
STATISTICS( | ||
selectedIcon = R.drawable.ic_statistics_filled, | ||
unselectedIcon = R.drawable.ic_statistics_outlined, | ||
tabName = "통계", | ||
), | ||
COMMUNITY( | ||
selectedIcon = R.drawable.ic_community_filled, | ||
unselectedIcon = R.drawable.ic_community_outlined, | ||
tabName = "투표", | ||
), | ||
MYPAGE( | ||
selectedIcon = R.drawable.ic_my_page_filled, | ||
unselectedIcon = R.drawable.ic_my_page_outlined, | ||
tabName = "마이페이지", | ||
), | ||
} |
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.
이 Enum Class는 특정 데이터를 담고 있기 때문에 desgin-system 모듈이 아닌 feature/navigator 모듈에 존재하는 것이 적합해보입니다!
val tabs = listOf( | ||
NavigationItem.SENT, | ||
NavigationItem.RECEIVED, | ||
NavigationItem.STATISTICS, | ||
NavigationItem.COMMUNITY, | ||
NavigationItem.MYPAGE, | ||
) |
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.
디자인 시스템의 컴포넌트는 다른 모듈에서 사용할 UI 껍데기라고 생각하시면 편할 것 같아요.
파라미터로 데이터를 입력하면 데이터를 표시하는 기능만을 수행해야 합니다!
그래서 이 탭에 대한 정보는 SusuNavigationBar
를 사용하는 곳에서 파라미터로 넘길 수 있도록 수정해주세요!
NavigationItem.MYPAGE, | ||
) | ||
|
||
var selectedBottomNavItem by remember { mutableStateOf(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.
이 상태도 SusuNavigationBar
를 사용하는 곳에서 선언하여 사용할 수 있도록 State Hoisting이 필요합니다! 컴포넌트 바깥에서 알 필요없는 특수한 상태가 아닌 경우에는 디자인시스템 컴포넌트 안에 상태가 있는 것은 어색합니다.
혹은 SusuNavigationBar
가 굳이 어떤 Item이 선택되었는지 알 필요가 없도록 구현하는 것도 방법입니다.
Material의 NavigationBar 구현을 살펴보시면 NavigationBar의 item들을 RowScope인 content로 받고 있습니다.
@Composable
fun NavigationBar(
modifier: Modifier = Modifier,
containerColor: Color = NavigationBarDefaults.containerColor,
contentColor: Color = MaterialTheme.colorScheme.contentColorFor(containerColor),
tonalElevation: Dp = NavigationBarDefaults.Elevation,
windowInsets: WindowInsets = NavigationBarDefaults.windowInsets,
content: @Composable RowScope.() -> Unit
)
그래서 아래처럼 RowScope인 람다 안에 NavigationBarItem
을 집어넣어서 사용하게 됩니다. 이렇게 되면 NavigationBar
는 어떤 컴포넌트든 RowScope로 담는 유연한 껍데기가 되겠지요?
NavigationBar
와 NavigationBarItem
구현과 구조를 참고해보세요!
NavigationBar {
tabs.forEachIndexed { index, data ->
NavigationBarItem(selected = , onClick = { /*TODO*/ }, icon = { /*TODO*/ })
}
}
.clickable( | ||
indication = null, | ||
interactionSource = remember { MutableInteractionSource() }, | ||
) { selectedBottomNavItem = index }, |
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/ui
모듈에 있는 진욱님이 만드신 Modifier.susuClickable
을 사용하시면 간결하게 ripple 여부 설정 가능하세요!
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.
헉 ... 제가 designsystem에 core:ui 의존성을 추가 안했네요 ...
designsystem의 build.gradle에 아래와 같이 추가 부탁드릴게요. 죄송함다 ㅜㅜ
@Suppress("DSL_SCOPE_VIOLATION") // TODO: Remove once KTIJ-19369 is fixed
plugins {
alias(libs.plugins.susu.android.library)
alias(libs.plugins.susu.android.library.compose)
}
android {
namespace = "com.susu.core.designsystem"
}
dependencies {
implementation(projects.core.ui)
}
private const val NAVIGATION_HEIGHT = 56 | ||
private const val DIVIDER_THICKNESS = 1 | ||
private const val TAB_COUNTS = 5 |
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.
Component의 옵션으로 쓰일만한 값들이 상수가 되면 안될 것 같아요 😥
네비게이션에 메뉴가 하나 더 생기거나 줄어들게 되면 이 상수를 찾아 수정해야만 합니다.
기본 값을 지정해야만 한다면 Component의 디폴트 파라미터로 옮깁시다!
@Composable | ||
fun SusuNavigation( | ||
modifier: Modifier = Modifier, | ||
) { | ||
Scaffold( | ||
bottomBar = { | ||
SusuNavigationBar() | ||
}, | ||
) { innerPadding -> | ||
Column( | ||
modifier = modifier | ||
.padding(innerPadding) | ||
.fillMaxWidth(), | ||
) {} | ||
} | ||
} |
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.
요 UI는 디자인시스템이 아닌 navigation을 구현할 feature/navigator 모듈에서 만들어 쓰면 됩니당.
디자인시스템에서는 삭제해도 될 것 같네여
@Composable | ||
fun SusuNavigationItem( | ||
modifier: Modifier = Modifier, | ||
item: NavigationItem, |
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.
이 Enum 파라미터도 컴포넌트가 NavigationItem
이라는 특정 Enum class를 몰라도 동작할 수 있도록 수정해주세요!
label: String,
@DrawableRes val selectedIcon: Int,
@DrawableRes val unselectedIcon: Int,
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 = item.tabName, | ||
style = SusuTheme.typography.title_xxxxs, | ||
color = if (selected) Gray100 else Gray40, | ||
modifier = modifier.padding(vertical = 3.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.
아 이거 ... 왜 넣으셨나했는데 제가 Typo에서 lineheight를 지정안해줬네요 ...
우선 해당 라인 제거 부탁드립니다! 버튼 작업하면서 LineHeight 작업도 같이 할게요~!
val configuration = LocalConfiguration.current | ||
val screenWidth = configuration.screenWidthDp.dp | ||
|
||
Column( | ||
modifier = modifier | ||
.width(screenWidth / TAB_COUNTS) | ||
.fillMaxHeight(), |
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에 weight(1f) 주는게 더 좋을거같아요.
예를 들어 screenWidth이 10이고 TAB_COUNTS가 3이라면 1dp의 크기가 누락됩니당.
fun SusuNavigationItem
-> fun RowScope.SusuNavigationItem
이렇게 바꾸면 Column에 weight줄 수 있을거에요
피드백 반영 완료했습니다! 확인 후에 다시 코드리뷰 남겨주시면 감사하겠습니다! |
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.
고생하셨습니다~ ㅎㅎ 👍
susuClickable은 제가 작업하면서 한번 달아볼게요~!
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.
피드백 반영 감사합니다 👍
💡 Issue
🌱 Key changes
NavigationBar(Material) + NavigationItem(custom)
이렇게 진행하려고 했었습니다.NavigationBar(custom) + NavigationBar(custom)
이렇게 진행하게 되었습니다.✅ To Reviewers
📸 스크린샷