-
Notifications
You must be signed in to change notification settings - Fork 8
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
[AN/USER] feat: 홈 화면 인기 축제 목록 구현 및 Presentation 모듈 분리(#656) #664
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
화면도 꽤 복잡하고 모듈 정리하는 것도 쉽지 않았을텐데 고생하셨습니다!! 👍
android:exported="true" | ||
android:screenOrientation="portrait"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세로 고정으로 가는군요ㅎㅎ 👍
class PopularFestivalViewPagerAdapter( | ||
context: Context, | ||
private val foregroundViewPager: ViewPager2, | ||
private val backgroundViewPager: ViewPager2, | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 ViewPager 2개가 같이 돌아가니까 복잡하네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimen을 사용하지 않는다면 ViewPagerAdapter에 context를 넘길 필요도 없을듯합니다! (리소스 접근 필요 없으니까)
private fun setViewPagersOffscreenLimit(limit: Int) { | ||
foregroundViewPager.offscreenPageLimit = limit | ||
backgroundViewPager.offscreenPageLimit = limit | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
popularFestivals의 개수는 제한적일테니 이렇게 offscreenPageLimit을 설정해도 크게 문제는 없을 것 같은데, 사실 양쪽으로 1개씩만 limit으로 설정해도 번쩍이는 문제는 해결할 수 있지 않나요?? 🤔
<dimen name="pageMargin">80dp</dimen> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 이제 dimen 사용하나요??ㅎ.ㅎ 어떤 경우에 사용하는 게 좋을지 의견 궁금해요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
코멘트만 확인 부탁드려요!
app:layout_constraintTop_toTopOf="@id/vpPopularFestivalBackground" /> | ||
|
||
<com.google.android.material.tabs.TabLayout | ||
android:id="@+id/into_tab_layout" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
카멜 케이스로 변경되어야 할 것같습니다!
import androidx.lifecycle.repeatOnLifecycle | ||
import kotlinx.coroutines.launch | ||
|
||
fun repeatOnStarted(lifecycleOwner: LifecycleOwner, action: suspend () -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util들은 common으로 가는 것이 어떤가요?
저희 최종 presentation layer가 하나로 존재할테니 임시로 중복을 허용해도 괜찮을라나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
근데 이 유틸이 presentation 에 한정적인 유틸이라 common 으로 가면 안될 것 같아요.
저는 임시로 중복을 허용하는게 좋다고 생각해요..!
android/festago/settings.gradle.kts
Outdated
include(":data-legacy") | ||
include(":presentation-legacy") | ||
include(":common") | ||
include(":presentation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include(":data-legacy") | |
include(":presentation-legacy") | |
include(":common") | |
include(":presentation") | |
include(":common") | |
include(":data-legacy") | |
include(":presentation") | |
include(":presentation-legacy") |
레이어를 분리하면서 정렬이 필요하다고 느껴지네요!
- 알파벳 순서
- 기능별로 정리
- 정리 없이 자동 추가에 의존한다.
어떤 방향성을 원하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별로 신경 안썼던 부분은데 기능별로 정리하는게 좋겠네요!
* | ||
* See [testing documentation](http://d.android.com/tools/testing). | ||
*/ | ||
class ExampleUnitTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 테스트 케이스는 제거해도 괜찮을 것같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 패키지가 안올라가서 그대로 두었습니다!
삭제하고 gitkeep 으로 변경할게요!
<?xml version="1.0" encoding="utf-8"?> | ||
<resources> | ||
|
||
<style name="H2Bold20Lh24"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디자이너 분들이라 그런지 네이밍 좋네요.
|
||
<item | ||
android:id="@+id/item_mypage" | ||
android:id="@+id/item_my_page" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:id="@+id/item_my_page" | |
android:id="@+id/itemMyPage" |
카멜 케이스로 바꿔야 할 것같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했는데 이걸 또...ㅜㅜ
|
||
</androidx.constraintlayout.widget.ConstraintLayout> | ||
|
||
<data> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
데이터를 가장 밑으로 내리신 이유가 있을까요?
|
||
<androidx.coordinatorlayout.widget.CoordinatorLayout | ||
<androidx.constraintlayout.widget.ConstraintLayout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기는 LinearLayout으로 변경하면 성능 상의 이점도 있고 코드도 더욱 간결해질 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에 Linear 로 만들었습니다. 간격 맞추기가 쉽지않아서 변경했습니다만 흠 가능할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
margin을 통해서 맞추면 가능 할 것같습니다만 이대로 진행해도 문제 없을 것 같습니다!
📌 관련 이슈
홈 화면 작업 끝나고 이슈를 닫는게 좋을 것 같네요.
#656
✨ PR 세부 내용
작업한 내용은 다음과 같습니다.
Presentation 모듈 분리
기존 presentation 을 presentation-legacy 로 변경하고
presentation 모듈을 새로 추가하였습니다.
모듈 분리 Commits
다크모드 와 라이트모드 통일
TextStyle, ColorStyle 정의
피그마에서 그대로 뽑아서 가져왔습니다.
[바텀 네비게이션 생성 및 프로젝트 세팅 Commits] (https://github.com/woowacourse-teams/2023-festa-go/pull/664/files/0113956b2773829c309f0cc56989bad805255a00..2ee1d10b911766dd59d1b74bba709542f39065a0)
인기 축제 목록 구현
각 뷰 사이 간격을 좁혀서 넘기기전에 이전, 다음 뷰를 볼 수 있습니다.
dimens 의 offsetBetweenPages (페이지간 좁히는 간격)와 pageMargin(페이지 양쪽 margin) 을 조작해 간격을 조절할 수 있습니다.
pageMargin (페이지 양쪽 간격) 의 두 배 보다 offset 이 크게 되면 화면이 보이기 시작합니다.
처음에 ImageView 로 만들었다가 foreground Item 이 select 되면 그떄 배경을 그리게 만들었는데 그러다보니 느려서 번쩍임이 발생했습니다.
그 다음으로 Glide 의 Preload 를 사용해 미리 캐싱하려했는데 캐싱 메모리 제한이 있는지 번쩍임은 사라지지 않았습니다.
그래서 ViewPager 의 offsetlimit 을 증가시켜 아이템 개수와 통일시켰습니다. 현재 보이지 않아도 미리 그려놓습니다.
PopularFestivalViewPagerAdapter 를 보면 두 ViewPager 를 관리하는 코드를 확인할 수 있습니다.
앱바 작성
앱바는 안드로이드 앱바를 쓰지 않고 constraint_layout 을 썼습니다.
축제 목록 화면 Commits
현재까지 완성된 화면
KakaoTalk_Video_2024-01-20-11-26-26.mp4