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

[Team 18-Android] 메인화면 구성하기 #9

Merged
merged 13 commits into from
May 26, 2022

Conversation

taewooyo
Copy link
Collaborator

@taewooyo taewooyo commented May 25, 2022

안녕하세요 앨런😀
이번 미션을 진행하게 된, @bn-tw2020 @shncder 입니다. 새롭게 jetpack compose로 레이아웃을 진행했습니다. 따끔한 리뷰에 대해서 흔쾌히 받아드려 수정해보도록 하겠습니다.

===========================================================================================

📑 구현

  • Jetpack Compose 을 이용해 레이아웃 구현

  • BottomBar 구현

  • TopBar 구현

    • TopBar 상태(CLOSED, OPEN)을 활용해 DefaultAppBar, SearchAppBar 분리해서 구현
    • DefaultAppBar는 메인 화면 보여주기
    • SearchAppBar는 검색 리스트 현황 보여주기
  • 여행지 리스트 구현

    • Lazy Row 내에서 Column으로 2줄로 구현
  • 메인 이미지 구현

    • Coil 라이브러리를 통해 구현
    • image state에 따라 Loading, Error, Success 처리
  • 숙소 리스트 구현

    • Lazy Row을 통해 Image, Text 출력
    • Image는 Coil 라이브러리 이용
메인 화면 검색 리스트 화면

🤔 질문사항

  • Bottom Navigation을 jetpack Compose을 통해 구현했습니다. xml을 통해서 디자인도 혼용해보고 싶은데 Composable 함수로 이동이 되서 xml을 못쓰게 되던데 어쩔 수 없는 부분인지 궁금합니다.

taewooyo and others added 13 commits May 24, 2022 11:07
바텀 네비게이션 구현
- jetpack compose을 통해 bottom bar 구현하기
feat: 바텀 네비게이션 구현(#2)
바텀 네비게이션 구현
- jetpack compose을 통해 bottom bar 구현하기
feat: 메인화면에 메인 이미지 넣기(#5)
이미지 로드전에 텍스트가 나오는 현상 수정
CircularProgressIndicator 가운데로 이동
상단 앱바 구현하기
- 상단 앱바의 상태를 OPEN, CLOSED 로 구별
- 상태에 따라 DeafultAppBar, SearchAppBar로 분리

여행지 리스트 구현하기
- viewmodel에서 더미 데이터를 만들어서 LazyRow로 진행
[FEAT] 상단 앱 바 구현 및 여행지 리스트 구현하기
숙소 이미지 및 정보 가져오기
 - CircularProgressIndicator 적용
 - 에러이미지 적용
 - 임시 데미 데이터 적용
메인화면 데이터 분리
- viewModel에서 데이터 관리 → repository로 분리
- 추후 비동기 통신을 위해 flow 적용
@taewooyo taewooyo self-assigned this May 25, 2022
@taewooyo taewooyo added the review-android Further information is requested label May 25, 2022
@taewooyo taewooyo requested a review from sabgilhun May 25, 2022 07:47
wooody92 pushed a commit that referenced this pull request May 25, 2022
refactor: 엔티티 수정및 추가
ink-0 pushed a commit that referenced this pull request May 25, 2022
Copy link

@sabgilhun sabgilhun left a comment

Choose a reason for hiding this comment

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

안녕하세요 bn-tw2020 shncder, 3주간 잘 부탁드립니다. 저도 도움이 될 수 있게 노력하겠습니다.

아쉽게도 저도 Compose를 많이 써보진 못해서 😢

제가 남긴 코멘트를 하나의 의견이라고 보시고 조금 가볍게 봐주세요 :)

질문답변

남겨주신 질문은 조금 찾아보니 AndroidView Composable이란게 있네요.

https://developer.android.com/reference/kotlin/androidx/compose/ui/viewinterop/package-summary?hl=ko#top-level-functions

위 내용이 도움이 되었길 빕니다.

추가

Compose를 사용한다면 공통으로 사용될 컴포넌트들을 정리하여 common/components를 빼내고 여러 화면에서 재사용할 수 있게 작업을 해보시는것도 좋은 경험이 될것 같아요.

ImageView같이 여러 곳에서 사용될 UI를 컴포넌트화 시켜보면 좋을것 같습니다!

id 'com.android.application'
id 'org.jetbrains.kotlin.android'
}

Choose a reason for hiding this comment

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

https://kotlinlang.org/docs/opt-in-requirements.html#module-wide-opt-in

공부를 위해 하는 프로젝트라, 베타 API 사용에 대한 warning 을 무시해도 될것 같습니다.

tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).all {
    kotlinOptions.freeCompilerArgs += ["-Xopt-in=coil.annotation.ExperimentalCoilApi"]
} 

이렇게 추가하면, ExperimentalCoilApi 어노테이션 붙이지 않아도 beta api 사용처에 warning 이 뜨지 않습니다.

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 +53 to +61
navController.graph.startDestinationRoute?.let { route ->
popUpTo(route) {
saveState = true
}
}

launchSingleTop = true
restoreState = true
}

Choose a reason for hiding this comment

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

popUpTo을 startDestinationRoute 대상으로 한 이유가 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

학습할 때 많은 분들이 저런식으로 이용해서 적용해봤습니다.
popUpTo는 BackStack에서 어디까지 이동할 것인지 결정하는 속성으로 알고 있습니다. 알맞은 설정인지 확인 후, 프로젝트에 적용해보겠습니다.

Comment on lines +19 to +29
topBar = {
MainAppBar(
searchWidgetState = searchWidgetState,
searchTextState = searchTextState,
onTextChange = { viewModel.updateSearchText(newValue = it) },
onCloseClicked = { viewModel.updateSearchText(newValue = "") },
onSearchClicked = {},
onOpenTriggered = { viewModel.updateSearchWidgetState(newValue = SearchWidgetState.OPEN) },
onCloseTriggered = { viewModel.updateSearchWidgetState(newValue = SearchWidgetState.CLOSED) },
)
},

Choose a reason for hiding this comment

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

MainAppBar가 '검색탭'에 특화되어 있는 AppBar로 보이네요.

계층을

MainScreen
ㄴ MainAppBar

MianScreen
ㄴ SearchScreen
ㄴㄴ MainAppBar

이런식으로 SearchScreen 안쪽으로 넣는건 어떨까요? 이렇게 하면 다른탭에선 앱바가 노출되지 않겠지만, MainAppBar 가 탭에 따라 동작을 달리하는 것도 그리 좋은 구조는 아닌것 같아서요.

혹시 다른 의견이 있다면 편히 말씀해주세요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋은 것 같아요. 적용해보도록 하겠습니다.



@Composable
fun DefaultAppBar(onSearchClicked: () -> Unit) {

Choose a reason for hiding this comment

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

외부에서 호출하지 않고, TopBar 내부에서 코드 모듈화를 위한 Composable 함수는 private로 숨기는 것은 어떨까요?

Comment on lines +100 to +111
fun handleImage(painterState: ImagePainter.State) {
if (painterState is ImagePainter.State.Loading) {
CircularProgressIndicator()
} else if (painterState is ImagePainter.State.Error) {
Image(
painter = painterResource(id = R.drawable.ic_error),
contentDescription = "Error Image",
modifier = Modifier.fillMaxSize(),
contentScale = ContentScale.FillBounds
)
}
}

Choose a reason for hiding this comment

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

Image Loading, Error 처리가 겹치고 있는것 같은데요. 아래처럼 공통화를 해보면 어떨까요?

@Composable
fun HandleImageResult(
    painterState: ImagePainter.State,
    onEmpty: @Composable (() -> Unit)? = null,
    onSuccess: @Composable (() -> Unit)? = null,
    onError: @Composable () -> Unit = { DefaultError() },
    onLoading: @Composable () -> Unit = { DefaultLoading() }
) {
    when (painterState) {
        is ImagePainter.State.Empty -> onEmpty?.invoke()
        is ImagePainter.State.Loading -> onLoading()
        is ImagePainter.State.Success -> onSuccess?.invoke()
        is ImagePainter.State.Error -> onError()
    }
}

@Composable
private fun DefaultLoading() {
    CircularProgressIndicator()
}

@Composable
private fun DefaultError() {
    Image(
        painter = painterResource(id = R.drawable.ic_error),
        contentDescription = "Error Image",
        modifier = Modifier.fillMaxSize(),
        contentScale = ContentScale.FillBounds
    )
}

}
if (index + 1 < travelLocations.size) {
Row {
LoadImage(location)

Choose a reason for hiding this comment

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

여기도 travelLocations[index + 1] 들어가야 할것 같습니다~


LazyRow(state = scrollState) {
itemsIndexed(travelLocations) { index, location ->
Column(modifier = Modifier.padding(end = 16.dp)) {

Choose a reason for hiding this comment

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

Item을 만드는 로직을 함수로 빼내면, 가독성이 좋아질것 같아요!

LazyRow(state = scrollState) {
    itemsIndexed(travelLocations) { index, location ->
        MakeColumn(...)
    }
}

@Composable
private fun MakeColumn(...) {
    Column(modifier = Modifier.padding(end = 16.dp)) {
        if (index % 2 == 0) {
            MakeItem(location)
            if (index + 1 < travelLocations.size) {
                MakeItem(travelLocations[index + 1])
            }
        }
    }
}

@Composable
private fun MakeItem(...) {
    Row {
        ...
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

적용하면 보다 개발자입장에서도 가독성이 좋은 것 같습니다.

dukcode added a commit that referenced this pull request May 26, 2022
* feat: Room의 int field Wrapper 클래스로 변경, Builder 추가

* chore: DB 방언 SQL로 변경

* feat: Entity auto-increment 적용

* feat: RoomRepository 구현

* test: RoomRepositoryTest 구현

* feat: RoomChargeDistributionResponse DTO 구현

* feat: RoomService getRoomChargeDistribution() 메서드 구현

* test: RoomService test 구현

* feat: Entity 생성자 parameter wrapper 클래스로 변경

* feat: responseDTO field final 제거 및 기본생성자 추가

* feat: HomeController 구현

* test: HomeController 테스트 구현
street62 pushed a commit that referenced this pull request May 26, 2022
kowoohyuk pushed a commit that referenced this pull request May 26, 2022
* Update README.md

* chore: 프로젝트 생성

* chore: styled-component 생성 및 절대경로 적용

* chore: 초기환경 설정

* feat: gnb 컴포넌트 생성 (#9)

* feat: searchBar 컴포넌트 마크업 구현 (#14)

* feat: home page 마크업 적용

* design: searchBar 마우스 커서 적용

* style: 인터페이스 이름 변경 (카멜케이스로)

* feat: 캘린더 컴포넌트 스타일 생성

* feat: 캘린더 컴포넌트 생성

* feat: 캘린더 모달 컴포넌트 스타일 생성

* 캘린더 모달 컴포넌트 생성

* style: globalStyle 파일 위치 변경

* feat: 캘린더 컴포넌트에 버튼 추가 및 지나간 날짜 비활성화 적용

* refactor: getTodayDate 함수 분리

* feat: 이전달, 다음달 이동 기능 구현

Co-authored-by: Seyeong Kim(Suntory) <seyoung7555@naver.com>
tmdgusya pushed a commit that referenced this pull request May 26, 2022
tmdgusya pushed a commit that referenced this pull request May 26, 2022
@sabgilhun sabgilhun merged commit 541b89d into codesquad-members-2022:team-18 May 26, 2022
sabgilhun pushed a commit that referenced this pull request May 26, 2022
[Android][히데]  홈 화면 UI 완성
SangHwi-Back pushed a commit that referenced this pull request May 27, 2022
[#3] fix: Tab 별 이미지와 색상을 요구사항에 맞게 수정
wnsxor1993 added a commit that referenced this pull request May 27, 2022
[iOS] refactor: SearchVC의 View 관련 수정
jminie-o8o pushed a commit that referenced this pull request May 27, 2022
…ge-recyclerview

[Android] 테마 적용 및 이미지 로더 및 RecyclerView 세팅
junseokseo9306 added a commit that referenced this pull request May 27, 2022
yeonnseok pushed a commit that referenced this pull request May 27, 2022
Accommodation 엔티티에 있던 accommodationImg 필드를 Info 엔티티로 옮기고, Main, DetailImg 로 분리
yeonnseok pushed a commit that referenced this pull request May 27, 2022
지금까지의 DB 요구사항 분석 및 ERD 다이어그램 추가
sanghyeok-kim added a commit that referenced this pull request May 28, 2022
- 네트워크 통신을 위한 프로퍼티를 갖는 EndPoint 설계 -> 추후 Alamofire로 리팩토링 예정

jeremy0405/airbnb/#9
sanghyeok-kim pushed a commit that referenced this pull request May 28, 2022
- Alamofire를 활용하도록 변경
- print() 부분 수정예정

jeremy0405/airbnb/#9
sanghyeok-kim added a commit that referenced this pull request May 28, 2022
- HTTPContentType, HTTPMethod 삭제 (Alamofire 사용)

jeremy0405/airbnb/#9
JasonLee0223 added a commit that referenced this pull request Jun 1, 2022
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- SearchHomeEntity
  - NearCity
  - HeroBanner

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- decode -> failToDecode
- invalidUrl 추가
- 그 외 case들은 불필요해서 삭제

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- Alamofire 활용한 request 메소드 구현

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- 모든 Repository들이 부모로 가질 상위 클래스 NetworkRepository (decdoe 메소드를 포함)
- SearchHome에서 사용할 Repository 프로토콜 (SearchHomeRepository)
- SearchHomeRepository을 채택해서 구현한 SearchHomeRepositoryImpl

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- URL을 받아 download task로 response 요청 후 UIImage 형태로 completion의 매개변수에 넣어줌
- Disk Caching 기능 구현

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- HeroBanner, NearCity 값을 API에서 받아온 data를 통해 출력하도록 변경함
- Theme도 마찬가지로 받아오도록 변경할 예정(현재 mock data 사용중)

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- VMAction, VMState에 선언된 accept, bind 메소드 삭제
  (해당 로직은 VM이 구현해도 되고 안해도 되므로)
- ImageManger 로직에 들어간 print문 삭제

jeremy0405/airbnb/#9
ITzombietux pushed a commit that referenced this pull request Jun 3, 2022
- SeachHomeEndPoint case 추가
- ThemeViewModel loadAction.bind 구문 변경
- SearchHomeRepositoryImpl/ requestTheme() 추가

jeremy0405/airbnb/#9
Dae-Hwa pushed a commit that referenced this pull request Jun 12, 2022
Feat: 위시리스트 추가 기능 구현
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-android Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants