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

Navigation 구현 #17

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Navigation 구현 #17

wants to merge 5 commits into from

Conversation

Gael-Android
Copy link

Summary

Navigation 입니다.
자세한 정보는 figma

https://www.figma.com/design/gvwhMF6iNkuYKzxipKzkaG/Handy-v1-(demo)?node-id=3897-22087&t=SA9mZOifEwjvSheS-1

를 참고해주시기 바랍니다.

Describe your changes

스크린샷 2024-11-22 오후 3 28 44

To reviewers

@yourssu/android-maintainer

@Gael-Android Gael-Android self-assigned this Nov 22, 2024
* @param label 아이템의 라벨
*/
data class BottomNavItem(
val icon: Int,
Copy link
Member

@HI-JIN2 HI-JIN2 Dec 16, 2024

Choose a reason for hiding this comment

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

저희 아이콘이 foundation에 정의되어 있어서 Handy에 있는 Icon을 사용하는게 좋지 않을까용? (이 시점에 머지가 안되었을 수도...) 거기 정의되어 있는 아이콘만 사용되지 않을까 하는.. 생각이 드네욥(요건 디자인팀한테 물어보기!)

icon: ImageVector
icon = HandyIcons.Line.InfoCircle,

근데 아이콘 말고도 라벨이 필요하니 Icon을 한번 감싸서 쓰는 방법도..?! 떠올라욥
지금 처럼 string으로 각각 인자 받으면 될 것 같슴당!

Copy link
Member

Choose a reason for hiding this comment

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

Int보다는 Handy 내 Icon만 사용할 수 있도록 하는 게 맞는 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

Icon을 사용할때 ImageVector가 아니라 Handy의 Icon 만을 가르키는 타입이 존재하나요?

Copy link
Member

@kangyuri1114 kangyuri1114 left a comment

Choose a reason for hiding this comment

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

리뷰 확인부탁드립니다!

* @param label 아이템의 라벨
*/
data class BottomNavItem(
val icon: Int,
Copy link
Member

Choose a reason for hiding this comment

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

Int보다는 Handy 내 Icon만 사용할 수 있도록 하는 게 맞는 것 같습니다!

*/
data class BottomNavItem(
val icon: Int,
val label: String
Copy link
Member

Choose a reason for hiding this comment

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

String? 으로 두어서 아이콘만 있는 navigation도 가능하도록 하면 좋을 것 같습니다

디자인팀에서도 아직 미정인부분 같긴한데 우선 코멘트 남겨두었어요
image

Comment on lines 49 to 50
modifier: Modifier = Modifier,
onClick: () -> Unit
Copy link
Member

Choose a reason for hiding this comment

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

순서바꾸면 좋을 것 같습니다!

Suggested change
modifier: Modifier = Modifier,
onClick: () -> Unit
onClick: () -> Unit,
modifier: Modifier = Modifier

Copy link
Author

Choose a reason for hiding this comment

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

넵!

@Composable
private fun BottomNavItem(
item: BottomNavItem,
isSelected: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

isSelected에 기본값이 있고 없고는 큰 상관이 없겠죠??

Copy link
Author

Choose a reason for hiding this comment

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

보통 false긴 한데 큰 상관없을듯합니다

Copy link
Author

Choose a reason for hiding this comment

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

Row(
            modifier = Modifier.fillMaxSize(),
            horizontalArrangement = Arrangement.SpaceBetween,
            verticalAlignment = Alignment.CenterVertically
        ) {
            items.forEachIndexed { index, item ->
                BottomNavItem(
                    item = item,
                    isSelected = index == selectedIndex,
                    modifier = Modifier.weight(1f),
                    onClick = { onItemSelected(index) }
                )
            }
        }

사용자가 지정할 일이 없고 상태가 저장되는 selectedIndex에 의해 선택 여부가 정해지기 때문입니다

Comment on lines 74 to 78
fontSize = 11.dp,
lineHeight = 16.dp,
fontWeight = FontWeight(400),
color = color,
textAlign = TextAlign.Center
Copy link
Member

Choose a reason for hiding this comment

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

typo를 활용해주시면 좋을 것 같습니다!
image

import com.yourssu.handy.compose.foundation.HandyTypography

Copy link
Author

Choose a reason for hiding this comment

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

넵!

.fillMaxWidth()
.height(56.dp),
) {
// Divider(thickness = Thickness.Thin) // Divider 추후 추가
Copy link
Member

Choose a reason for hiding this comment

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

divider 머지 후 이부분도 수정 확인 위해서 코멘트 남겨두겠습니다

Copy link
Member

@kangyuri1114 kangyuri1114 left a comment

Choose a reason for hiding this comment

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

이부분도 답변 부탁드립니다~

selectedIndex: Int,
onItemSelected: (Int) -> Unit
) {
require(items.size in 3..5) { "Items size must be between 3 and 5" }
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 items수를 강제하는 건 좋은 것 같긴한데 require를 사용하게 되면
해당 컴포넌트로 개발 진행 시에 item수 미달, 초과로 preview단에서 오류가 뜨면 추적이 어려울 것 같은데 어떻게 생각하시나요??
item을 6개 넣으면 preview render오류가 뜨긴하지만 메시지는 보이지 않네요,,
image

Copy link
Author

Choose a reason for hiding this comment

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

image
이슈 패널 열면 뜨고 에뮬레이터에 돌리면 에러가 납니다.

@kangyuri1114 kangyuri1114 mentioned this pull request Jan 12, 2025
@Gael-Android
Copy link
Author

.idea/deploymentTargetSelector.xml 가 conflict가 나는데 이게 뭘까요

@Gael-Android Gael-Android force-pushed the gael/feature/navigation branch from 1195208 to 127b8bc Compare January 14, 2025 08:23
@Gael-Android
Copy link
Author

@kangyuri1114 수정완료했습니다~

Copy link
Member

Choose a reason for hiding this comment

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

요 파일은 삭제 해도 될 것 같습니다~

Copy link
Author

Choose a reason for hiding this comment

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

넵!

Copy link
Author

Choose a reason for hiding this comment

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

제거했습니다

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

Successfully merging this pull request may close these issues.

3 participants