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

[CAT-140] MnTopAppBar 구현 / Icon LocalContentColor로 수정 #42

Merged
merged 6 commits into from
Aug 3, 2024

Conversation

lee-ji-hoon
Copy link
Collaborator

작업 내용

M3 TopAppBar를 사용하려다가 ExperimentalMaterial3Api 이라서 그런지 불안전해보여서 내부 코드 조금 보면서 내가 그냥 Layout 만들었어

figma-link

MnTopAppBar 구현했으며 3가지를 고려

  1. Title만 있는 경우
  2. NavigationIcon만 있는 경우
  3. Actions만 있는 경우

체크리스트

  • 빌드 확인

동작 화면

image

살려주세요

@lee-ji-hoon lee-ji-hoon added the feature 기능 개발 label Aug 2, 2024
Copy link

linear bot commented Aug 2, 2024

Comment on lines +48 to +53
Box(Modifier.layoutId(ACTIONS)) {
CompositionLocalProvider(
value = LocalContentColor provides topAppBarColors.actionIconContentColor,
content = actions
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

여기하다가 느낀건데 #38 (comment) 이 코멘트 번복해도 될까 👀

막상 컴포넌트 만들면서 Color들 좀 사용 편하게 해주려고 하니까 LocalContentColor를 provide 해주고 그것을 컴포넌트에서 쓰는게 편한거 같아서~!

Copy link
Member

Choose a reason for hiding this comment

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

provide 하는 방식으로 구현하면 LocalContentColor 이렇게 바꾸는게 맞을 것 같네요~~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그럼 내가 슬쩍 반영해둬도 될까?🧎‍♂️

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
Collaborator Author

Choose a reason for hiding this comment

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

그럼 다른 리뷰 없으면 저거만 수정하고 머지할게!

Comment on lines +93 to +103
Box(
modifier = Modifier
.size(40.dp)
.noRippleClickable { },
contentAlignment = Alignment.Center
) {
MnMediumIcon(
resourceId = R.drawable.ic_null,
tint = topAppBarColors.navigationIconContentColor
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IconButton 이라는 선택지도 있는데 M3 Iconbutton은 최소 크기 및 start padding을 내부에서 주는게 있어서 사용이 생각보다 까다로워서 우선 안썼고 직접 만들까 고민중이야

Copy link
Member

@HyomK HyomK Aug 2, 2024

Choose a reason for hiding this comment

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

아 이거 맞아 padding 디자인 스펙이라 다르면 불편함... 아니면 나 버튼 작업에 끼워 넣을까?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

시간되면 하나 만들어두먼 좋을거 같아!

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
Collaborator Author

Choose a reason for hiding this comment

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

엉 고마워~!

@HyomK
Copy link
Member

HyomK commented Aug 2, 2024

내부 코드 가져왔다고 해서 나도 기본 코드 봤는데 잘 가져왔다 👍

@lee-ji-hoon lee-ji-hoon changed the title [CAT-140] MnTopAppBar 구현 / Icon Ima [CAT-140] MnTopAppBar 구현 / Icon LocalContentColor로 수정 Aug 3, 2024
Comment on lines +29 to +42
@Composable
fun MnMediumIcon(
imageVector: ImageVector,
modifier: Modifier = Modifier,
contentDescription: String? = null,
tint: Color = LocalContentColor.current
) {
Icon(
imageVector = imageVector,
contentDescription = contentDescription,
modifier = modifier.size(MnIconSize.medium),
tint = tint
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@HyomK tint 수정하면서 ImageVector 객체를 받는 MnIcon도 추가했어~!

@lee-ji-hoon lee-ji-hoon merged commit a779e08 into develop Aug 3, 2024
1 check passed
@lee-ji-hoon lee-ji-hoon deleted the feature/cat-140 branch August 3, 2024 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants