-
Notifications
You must be signed in to change notification settings - Fork 2
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] OnBording 뷰 구현 #17
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.
아주 빠르시군여 고생하셨습니당 !! 건강한 자극 감사합니다 열시미 할게요
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" /> | ||
|
||
<include |
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 활용 좋네요!! 배워갑니당.
android:layout_width="0dp" | ||
android:layout_height="wrap_content" | ||
android:layout_marginHorizontal="38dp" | ||
android:layout_marginTop="@dimen/spacing9" |
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 활용은 아직 어색한데 이거 보구 함 잘 써보겠습니다~!
app:layout_constraintTop_toTopOf="@id/include_onbording_group_original" | ||
app:title="@{@string/on_bording_new_group}" /> | ||
</androidx.constraintlayout.widget.ConstraintLayout> | ||
</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.
요기 린트체크 오류나서 한 줄 띄워주시면 좋을 것 같아욤!
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.
엇... 마지막줄 띄우는 린트 에러는 kt파일에서만 적용되는거 아니였나여..?
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.
.kt 파일에만 적용되는 거 맞습니다 !
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.
제가 배경 색상 잘못 설정해두었네요 ㅋㅋ 다음 pr에 수정해서 올릴테니 일단 배경색 무시하고 작업해주세요 (컴포넌트마다 설정해둔 배경색 모두 지워주세요!)
수고하셨습니다 !
<androidx.constraintlayout.widget.ConstraintLayout | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:background="@color/g_11" | ||
tools:context=".presentation.ui.onbording.OnBordingActivity"> | ||
|
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.
전체적으로 뷰 양 옆 공백이 24dp죠?? 가이드 라인을 이용해주는 게 더 좋을 것 같습니당 !! 제가 어제 공유한 아티클 참고하셔요 !
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.
가이드라인 사용해본적이 없어 한 번 사용해보겠습니다.
근데 궁금한 점이 있습니다!!!
가이드라인을 사용하게 되면 코드가 더 길어지게 되는데 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.
저는 나중에 유지 보수 측면에서 이점이 있다고 생각하기 때문에 가이드라인을 자주 사용하는 편입니다. 보통 디자이너 분들이 넘겨주시는 뷰를 보면 좌우로 일정한 여백값을 남겨두는 경우가 많아요. 만약 나중에 이 여백 값이 전부 수정될 때 가이드 라인을 적용해 두었으면 가이드라인의 end나 begin 값만을 수정해주면 되지만 가이드 라인을 적용해두지 않았다면 컴포넌트 하나하나 마진 값을 수정해 주어야 하니까요!
<androidx.constraintlayout.widget.ConstraintLayout | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:background="@color/g_11" |
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.
제가 background 색상 설정해두어서 따로 설정 안 해도 되지 않나요???
android:layout_height="wrap_content" | ||
android:layout_marginStart="24dp" | ||
android:layout_marginTop="147dp" | ||
android:background="@color/g_11" |
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.
전체적으로 background 없애주셔도 될 것 같습니다
android:id="@+id/tv_on_bording_title" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginStart="24dp" |
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:layout_marginStart="24dp" | |
android:layout_marginStart="@dimen/spacing24" |
app/src/main/res/values/strings.xml
Outdated
|
||
<!-- OnBording --> | ||
<string name="on_bording_title">핑글에 오신걸\n환영합니다!</string> | ||
<string name="on_bording_original_group">기본 단체\n입장하기</string> |
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.
<string name="on_bording_original_group">기본 단체\n입장하기</string> | |
<string name="on_bording_original_group">기존 단체\n입장하기</string> |
ㅋㅋ
<variable | ||
name="title" | ||
type="String" /> |
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.
기존 단체 입장과 신규 단체 개설을 type을 통해 관리하는 게 어떨까요? 후에 그래픽이 달라질 것 같은데 그렇게 되면 title과 그래픽 이미지를 따로 바인딩 시켜주는 것보다 타입을 바인딩 시켜주는 게 더 나아 보여서요!
<include | ||
android:id="@+id/include_onbording_group_original" | ||
layout="@layout/view_on_bording_group" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginStart="@dimen/spacing24" | ||
android:layout_marginTop="@dimen/spacing34" | ||
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toBottomOf="@id/tv_on_bording_title" | ||
app:title="@{@string/on_bording_original_group}" /> | ||
|
||
<include | ||
android:id="@+id/include_onbording_group_new" | ||
layout="@layout/view_on_bording_group" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginStart="@dimen/spacing8" | ||
android:layout_marginEnd="@dimen/spacing24" | ||
app:layout_constraintBottom_toBottomOf="@id/include_onbording_group_original" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintStart_toEndOf="@id/include_onbording_group_original" | ||
app:layout_constraintTop_toTopOf="@id/include_onbording_group_original" | ||
app:title="@{@string/on_bording_new_group}" /> | ||
</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.
기기대응을 생각해보면 두 include를 체인으로 연결해주는 게 낫겠다는 생각이 드는데,,, 어떻게 생각하시나요?
app:title="@{@string/on_bording_original_group}" /> | ||
|
||
<include | ||
android:id="@+id/include_onbording_group_new" |
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/include_onbording_group_new" | |
android:id="@+id/include_on_bording_group_new" |
네이밍의 통일성을 지켜주세요. 다른 것들도 한 번 씩 확인 부탁해요
android:id="@+id/iv_on_bording_group" | ||
android:layout_width="0dp" | ||
android:layout_height="96dp" | ||
android:layout_marginHorizontal="@dimen/spacing28" |
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.
여기도 기기 대응을 생각하면 마진 값을 주지 않는 게 나아보이네요.
정리하자면,,, 여기에 마진값을 주지 않고, activity 부분에서 두 include 간 체인을 걸어주고, include의 width를 잘 조절하면 (어떻게 조절해야 할까요? 고민해보세요 ㅋㅋ) 기기 대응에 더 유리할 것 같다는 생각입니다. (화면이 양 옆으로 넓은 경우를 생각해보세요!)
app/src/main/AndroidManifest.xml
Outdated
@@ -16,11 +16,13 @@ | |||
android:theme="@style/Theme.Pingle" | |||
android:usesCleartextTraffic="true" | |||
tools:targetApi="31"> | |||
<activity | |||
android:name=".presentation.ui.onbording.OnBordingActivity" | |||
android:exported="false" /> |
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="false" /> | |
android:exported="false" | |
android:screenOrientation="portrait" | |
tools:ignore="LockedOrientationActivity" /> |
액티비티 생성 시 항상 넣어주세요! 그리고 이 부분 더미 액티비티 아래 쪽으로 가게 해주세요 ~
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.
형님 이거 죄송한데 activity 먼저 쓰고 meta-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.
고생했어용! 요거 하나랑 온보딩 네이밍 전체적으로 한 번만 고쳐주고 머지합시둥!
<include | ||
android:id="@+id/include_onbording_group_new" | ||
layout="@layout/view_on_bording_group" | ||
android:layout_width="wrap_content" |
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:layout_width="wrap_content" | |
android:layout_width="0dp" | |
android:layout_marginEnd="@dimen/spacing8" |
기기대응을 생각했을 때 이게 크기가 고정되어 있는 것보다 가운데 공백 값을 유지시켜주는 게 좋겠죠? 그래서 0dp를 사용하는 게 좋아 보입니다. 아래도요! (근데 마진은 하나에만 넣어주면 될 것 같아요 !)
Related issue 🛠
Work Description ✏️
Screenshot 📸(업데이트!)
Uncompleted Tasks 😅
To Reviewers 📢
궁금점! 힐트로 인해서 뷰마다 AndroidEntryPoint 어노테이션을 작성하는데 온보딩의 뷰같은 경우 따로 의존성주입이 필요 없다고 생각되어서 어노테이션을 작성하지 않았는데 필수적으로 있어야하는건가요?