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

[feat] [#38] 부스 화면 구현 #44

Merged
merged 28 commits into from
Apr 10, 2024
Merged

Conversation

wjdtkdgns777
Copy link
Collaborator

스크린샷 2024-04-05 171801

@wjdtkdgns777 wjdtkdgns777 added the design tasks releated to design label Apr 5, 2024
This was linked to issues Apr 5, 2024
@easyhooon easyhooon removed a link to an issue Apr 5, 2024
wjdtkdgns777 and others added 6 commits April 5, 2024 17:49
navigationIcon 화면에 따라 변경 가능하도록
Nested Navigation 구현, Map 화면과 연결, BoothId 를 전달
UiState 를 통해 화면내에 데이터를 관리, BoothLocationScreen 생성
@easyhooon easyhooon marked this pull request as draft April 5, 2024 11:01
@easyhooon easyhooon marked this pull request as ready for review April 8, 2024 12:42
@easyhooon easyhooon added the enhancement New feature or request label Apr 8, 2024
fun BoothLocationScreen(
uiState: BoothUiState,
onBackClick: () -> Unit,
latitude: Double,
Copy link
Collaborator

Choose a reason for hiding this comment

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

uiState만 전달해도 될거 같아여

@Preview
@Composable
fun BoothLocationScreenPreview() {
BoothLocationScreen(
Copy link
Collaborator

@easyhooon easyhooon Apr 8, 2024

Choose a reason for hiding this comment

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

UnifestTheme 로 감싸주시구

@easyhooon
Copy link
Collaborator

현재 BoothDetail 화면 Figma BoothDetail 화면

Bookmark 관련해서 아이콘 높이조절이랑 폰트 사이즈조절이 필요합니다

@easyhooon
Copy link
Collaborator

easyhooon commented Apr 8, 2024

image

상단에 Booth Image가 figma 처럼 전체영역(statusbar, 좌우)를 차지하지 않고 있는데 이건 제가 해볼게요

@easyhooon
Copy link
Collaborator

easyhooon commented Apr 8, 2024

그 mainScreen 보시면 Scaffold 로 전체 영역을 그리고 있거든요. 그래서 그 내부에서 또 중첩으로 Scaffold를 사용하는게 그닥 좋지 않은 것 같긴합니다. 아마 BoothDetail 화면에서의 Snackbar 호출과 bottomBar 를 사용하기 위해 그렇게 Scaffold로 짜신거같은데, 제가 볼땐, Snackbar 이벤트는 Main 모듈로 전달해서, 최상단 Main 에 있는 Snackbar 를 통해 호출을, BottomBar 는 Box나, Card로도 충분히 구현할 수 있을 것 같습니다. (MainScreen 에 있는 onShowSnackbar 를 전달 -> 전달 -> 전달 해서 사용하는 곳에서 호출 하라는 의미)

image

그리고 Scaffold 를 사용하면 다음과 같이 innerPadding 에 의해 내부 content 가 bottomBar, TopBar 에 겹치지않도록 위아래 패딩이 주어지는데, 따라서 현재 처럼 topAppBar 를 두면 저렇게 아래에 위치하게 됩니다.(물론 다른 modifier를 쓰면 해결할수있긴한데) Scaffold 를 쓸땐 topBar 는 topAppBar 위치에 두어야해요. 아니면 위에 제가 말씀드린것처럼 그냥 Scaffold 말고 Box로 전체영역을 감싸는게 좋을듯 합니다.

https://github.com/Nexters/ilab-android/blob/develop/feature/createimage/src/main/kotlin/com/nexters/ilab/feature/createimage/CreateImageCompleteScreen.kt

여기 코드 참고하면 좋을거같고
WindowInsets 를 활성화하는 것으로 SystemBar 영역까지 App에서 사용할 수 있는 영역을 확장할 수 있는데, 그것을 적용하고 있는 것입니다. 공식문서 한번 읽어보시는거 추천드려요

에러를 처리하기 위한 스낵바가 아닌, 메세지를 출력하기 위한 스낵바로 그 용도를 변경
@easyhooon
Copy link
Collaborator

easyhooon commented Apr 8, 2024

지금 봤을땐 BoothDetail 화면의 SystemBar 내에 icon들 (현재 시각, wifi, data, 알림)이 흰색으로 판단되는데, 사진자체가 밝은 계열일 경우 이게 안보일 수 가 있습니다. 그러면 사진의 색상 추출하여, 이를 통해 SystemBar 영역의 색상을 분기처리 해야 하는건가 싶은데... 개발자에게 너무 힘든 디자인이네요, 디자인에 문제가 있는 것으로 판단됩니다.

보통 SystemBar 까지 전부 사용하는 앱의 경우 화면 전체 테마에 따라 동일한 색감으로 유지하는 것으로(다크계열, ex)Youtube Music, 단색 계열, ex) 카카오뱅크)
위와 같은 문제를 해결하는 편인데, 그런 고려가 되어있지 않는 것 같습니다.

호은님이 왜 바텀시트로 구현해서 StatusBar 영역까지 화면이 차지하지않게 구현한지 알것같은... 뒤로가기 버튼 색상 관련 문제도 그렇구

}
item { Spacer(modifier = Modifier.height(20.dp)) }
item { MenuText() }
items(uiState.boothDetailInfo.menus) { menu ->
Copy link
Collaborator

@easyhooon easyhooon Apr 8, 2024

Choose a reason for hiding this comment

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

LazyColumn 에서 items 를 사용할때는 반드시 key를 지정해줘야 합니다. 그래야 무분별한 recomposition 을 막을 수 있어요
-> 생각해보니깐 뭔가 이상태에서 리스트내에 아이템들이 업데이트가 일어날 것 같진 않아보이는군요. 일반적으로 데이터의 변동을 염두해두기에 key를 지정하면 좋다고 말씀드리려고 했는데 '반드시 key를 지정해야만 한다.'는 아니네요

)
Spacer(modifier = Modifier.height(3.dp))
Text(
text = "${menu.price}원",
Copy link
Collaborator

Choose a reason for hiding this comment

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

세자리씩 끊어서 , 처리하는 코드가 필요합니다. 확장함수나 유틸함수로 하나 만들면 좋을거같은데 gpt 나, Copilot 한테 부탁하면 아마 잘 만들어줄겁니다.

* Extends Int to format as a currency string with comma separators.
*/
fun Int.formatAsCurrency(): String {
return "%,d원".format(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

먼가 파이썬 스럽군요 이런식으로도 만들수있구나

Copy link
Collaborator

@easyhooon easyhooon left a comment

Choose a reason for hiding this comment

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

LGTM! 수고하셨습니다 snackbar 를 main 에 있는 것을 가져와서 쓰면서 snackbar 가 노출될때 하단의 버튼들을 가려버리는 문제가 있는데 이는 snackbar 가 노출되는 위치를 커스텀해야하나 싶어서, booth feature랑 크게 관련은 없어, 제가 main 부분 만질때 고쳐보겠습니다

@wjdtkdgns777 wjdtkdgns777 merged commit 6e00826 into develop Apr 10, 2024
1 check passed
@wjdtkdgns777 wjdtkdgns777 deleted the feature/booth-screen-ui branch April 10, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design tasks releated to design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Booth 화면 UI
2 participants