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

[ADD/#15] TopAppBar #17

Merged
merged 12 commits into from
Jul 7, 2024
Merged

[ADD/#15] TopAppBar #17

merged 12 commits into from
Jul 7, 2024

Conversation

arinming
Copy link
Contributor

@arinming arinming commented Jul 6, 2024

⛳️ Work Description

  • TerningTopAppBar 구현
  • BackButtonTopAppBar
  • LogoTopAppBar
  • MyPageTopAppBar

📸 Screenshot

15.mp4

📢 To Reviewers

  • TerningTopAppBar가 기본 탑 바이고, 나머지가 구체적으로 구현한 탑 바 입니당 아무것도 없는 앱바는 TerningTopAppBar 쓰시면 돼요! 영상 속 캘린더앱바가 TerningTopAppBar입니당.
  • 프로필 수정 부분은 ,,,, 피그마와 다르게 아이콘 자체에 마진이 들어가 있어서 일단 저렇게 해두었는데 추후 디자인쌤들과 논의해보는 게 좋을 것 같아용
  • 로고는 아직 추가 안해둔 상태입니당
  • BackButtonTopAppBar의 경우 title이 있고, 없는게 있어서..! 타이틀 없는 경우엔 ""로 넘겨주세욥~~~~!
  • 아 맞다 ㅜ drawable 임포트 이슈로,,, core에서 구현하기 보다 feature에 필터링 했는데 이 부분 어떻게 생각하시나여........!!!! 많은 조언 부탁

@arinming arinming added ADD ➕ 부수적인 코드 추가 및 라이브러리 추가, 새로운 파일 생성 FEAT ✨ 새로운 기능 구현 아린💛 아린 labels Jul 6, 2024
@arinming arinming added this to the 프로젝트 초기 세팅 milestone Jul 6, 2024
@arinming arinming self-assigned this Jul 6, 2024
@arinming arinming linked an issue Jul 6, 2024 that may be closed by this pull request
3 tasks
Copy link
Member

@Hyobeen-Park Hyobeen-Park left a comment

Choose a reason for hiding this comment

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

진짜 이렇게 빠르게 만들었는데 이렇게 깔끔할수가 있다고요???? 이게 터닝안드지~ 수고하셨습니당🥰

text = title,
modifier = Modifier.align(Alignment.Center),
textAlign = TextAlign.Center,
style = TerningTypography().title2
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
Contributor Author

Choose a reason for hiding this comment

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

무지성으로 TerningTypography 자동완성해서 임포트했는데 되더라구요,,,헐! ㅠ

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

Choose a reason for hiding this comment

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

예리하당! 고쳤습니당 근데 저 right 버튼이 캘린더에는 24사이즈로 있는 것 같아서 석준오빠랑 얘기함 해봐야할 것 같긴 하네용,,

Copy link
Member

@boiledEgg-s boiledEgg-s left a comment

Choose a reason for hiding this comment

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

빠르다 빨라,,, 너무 고생하셨습니당!!

Copy link
Member

@leeeyubin leeeyubin left a comment

Choose a reason for hiding this comment

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

진짜 속도 최고다,,, 너무 잘하는데요!!!!! 든든한 아린언니,,
drawable 임포트 문제는 더 이야기 해보도록 해요!!!

Comment on lines 39 to 47
topBar = {
when (navigator.currentTab) {
MainTab.HOME -> LogoTopAppBar()
MainTab.CALENDAR -> TerningTopAppBar()
MainTab.SEARCH -> LogoTopAppBar()
MainTab.MY_PAGE -> MyPageTopAppBar()
null -> LogoTopAppBar()
}
},
Copy link
Member

Choose a reason for hiding this comment

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

혹시 MainScreen에서 탑바를 넣어준 이유가 있을까용,,?

바텀바는 깜빡임을 방지하기 위해 보이는 부분과 안 보이는 부분을 나눴었는데
이 탑바들은 각 screen마다 관리해줘도 될 것 같아서요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MainScreen에서 Scaffold로 네 개의 화면을 감싸주고 있는데 해당 Scaffold의 topbar 속성에 한 번에 적용하려고 했습니당
각 screen마다 관리를 해주게 되면 Screen마다 Scaffold로 한 번 더 감싸서 topbar로 넣어주어야 하는 걸로 알고 있어요..!
그러면 메인스크린+각스크린 해서 Scaffold가 두번 감싸지게 되어서,, 일단 저렇게 구현해보았습니당.

그.. Column에 바로

`
@composable
fun HomeScreen() {
Column(modifier = Modifier.fillMaxSize()) {
LogoTopAppBar()
Text(text = "홈 스크린")
}
}

`

이렇게 때려넣으면

image

요롷게 위에가 붕뜨는 현상이.,,. ㅠㅜ

Copy link
Contributor Author

@arinming arinming Jul 7, 2024

Choose a reason for hiding this comment

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

예를 들어서 유빈이가 만든 signin 스크린은 바텀네비가 없고 메인스크린에서 관리되는 애가 아니니까 따로 Scaffold로 감싸서 topbar를 넣어주면 되는데 메인 스크린에서 바텀네비로 관리되는 애들은 scaffold로 두번 감싸기 or 메인에서 관리하기
로 해야할 것 같은데 어떻게 생각하는징!?!

더 좋은 방법을 아직 내 머리론 찾지 못했다 ㅜ ㅜ

Copy link
Member

Choose a reason for hiding this comment

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

카톡방에서 이야기 나눈대로 Scaffold로 한 번 더 감싸서 관리하도록 해요!!
해보구 불편한 점 발견되면 수정해보는 걸로 합시다!
탑바 관련해서 수정되면 한 번 더 알려주세용!

Comment on lines 25 to 26
onBackButtonClick: (() -> Unit)? = null,
) {
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
onBackButtonClick: (() -> Unit)? = null,
) {
onBackButtonClick: () -> Unit = {},
) {

이런 식으로 작성하면 null 체크를 따로 안 해줘도 될 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

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

확실하진 않지만 customActions도 비슷하게 바꿔줄 수 있을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

체고야,, customActions까지 바로 적용해씁니다

Comment on lines 38 to 56
navigationIcon = {
if (showBackButton) {
IconButton(onClick = {
onBackButtonClick?.invoke()
}) {
Icon(
painter = painterResource(id = R.drawable.ic_back),
contentDescription = stringResource(id = R.string.ic_back)
)
}
} else {
customActions?.getOrNull(0)?.invoke()
}
},
actions = {
customActions?.drop(1)?.forEach { customAction ->
customAction()
}
},
Copy link
Member

Choose a reason for hiding this comment

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

오! 이 속성들이 궁금해서 찾아봤는데 navigationIcon는 앱바의 왼쪽에 위치하는 아이콘을, actions는 오른쪽에 위치하는 아이콘의 액션을 나타내는 거군여,,,,,,??!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞아용! Tmi를 덧붙이면,,, 기본 TopAppBar로 구현했을 때 navigationIcon, actions 중에 하나만 있는 경우 타이틀이 가운데 정렬되지 않고 아이콘 사이즈 만큼 왼쪽, 또는 오른쪽으로 치우치는 이슈가 있더라구요 ㅜㅜ 그래서 TopAppBar대신 타이틀을 중앙으로 정렬해주는 CenterAlignedTopAppBar 적용해주었삽니다.

Comment on lines 19 to 21
customActions = listOf(
{},
{
Copy link
Member

Choose a reason for hiding this comment

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

여기 빈 { }가 생기는 이유는 TerningTopAppBar에서 customActions의 인덱스1부터 실행되게 했기 떄문일까요..?
혹시 customActions에 대해 설명해주실 수 있을까요,,ㅜㅜ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

마이페이지는 위와 같이 '프로필 수정' 이라는 텍스트 + 오른쪽 아이콘 으로 이루어져있고

스크린샷 2024-07-07 오후 5 14 05

로고 탑 바는 navigation이 따로 없는 이미지..?! 형식의 아이콘임을 확인할 수 있는데

navigationIcon 자리에는 BackButton이 기본으로 들어가는 게 직관적일 것 같아서, 로고를 navigationIcon에 넣기보다는 actions으로 빼야겠다는 생각을 했습니당.

그래서 LogoTopAppBar의 액션에는 list 첫번째에 바로 로고 이미지를 넣고, 액션 리스트의 첫번째 컴포저블은 탑 바의 왼쪽 (네비게이션아이콘 자리) 에 들어갈 수 있게 해두었습니다!
액션 리스트의 두번째 컴포저블부터는 타이틀의 왼쪽에 순서대로 배치되는 형식입니다. 마이페이지에는 왼쪽에 로고가 없어서 첫번째 컴포저블을 {}로 둔 것입니당.!

Copy link
Member

@leeeyubin leeeyubin Jul 7, 2024

Choose a reason for hiding this comment

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

자세한 설명 감사합니다! 탑바 적용할 때 참고해서 개발하도록 할게요!

@arinming arinming requested a review from leeeyubin July 7, 2024 08:36
Copy link
Member

@leeeyubin leeeyubin left a comment

Choose a reason for hiding this comment

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

수정된 부분 확인했습니다~!!

@arinming arinming merged commit 8302624 into develop Jul 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADD ➕ 부수적인 코드 추가 및 라이브러리 추가, 새로운 파일 생성 FEAT ✨ 새로운 기능 구현 아린💛 아린
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[ADD] TopAppBar
4 participants