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] 회원가입 뷰 구현 #16

Merged
merged 55 commits into from
Jul 17, 2023
Merged

[feat] 회원가입 뷰 구현 #16

merged 55 commits into from
Jul 17, 2023

Conversation

jooyyoo
Copy link
Member

@jooyyoo jooyyoo commented Jul 11, 2023

Related issue 🛠

Work Description ✏️

  • 회원가입 뷰 디자인

Screenshot 📸

feat-signup-view.mp4
default.mp4

Uncompleted Tasks 😅

  • 바텀시트
  • ◀️ 버튼 클릭 시 로직 구현

To Reviewers 📢

사랑해 안빵티비들

@jooyyoo jooyyoo added this to the 뷰 작업 milestone Jul 11, 2023
@jooyyoo jooyyoo self-assigned this Jul 11, 2023
@jooyyoo jooyyoo changed the title Feat signup view [feat] signup 뷰 구현 Jul 11, 2023
Copy link
Member

@Dan2dani Dan2dani left a comment

Choose a reason for hiding this comment

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

수고하셨습니당 ~~

Comment on lines 11 to 12
class DummyActivity : BindingActivity<ActivityDummyBinding>(R.layout.activity_signup_email) {
private val viewModel: DummyViewModel by viewModels()
Copy link
Member

Choose a reason for hiding this comment

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

그리고 지금보니까 왜 엑티비티들이 하나도 메니페스트에 추가가 안돼있을까용?

android:id="@+id/img_back_arrow"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginLeft="5dp"
android:layout_marginTop="34dp"
android:background="@null"
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
Member Author

Choose a reason for hiding this comment

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

이거 꼭 있어야 하나용?!

첨에 코드리뷰 보고 없앴더니 디자인이랑 달라지더라고여ㅜ

android:layout_marginTop="34dp"
android:background="@null"
android:layout_marginLeft="17dp"
Copy link
Member

Choose a reason for hiding this comment

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

android:layout_marginStart="17dp"
로 수정해주세영 그리고 이거 패딩 값이 생겨서 이 마진 값이 아닐거 같긴한뎅 확인 부탁~~~

@@ -45,25 +45,22 @@
android:id="@+id/tv_sign_up_page_number"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="34dp"
android:layout_marginRight="24dp"
android:padding="12dp"
Copy link
Member

Choose a reason for hiding this comment

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

이건 패딩이 없어도 될 거 같은데용~ 터치영역과 관련된 부분이 아니기 때문에!


<TextView
android:id="@+id/tv_input_email_phrase"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginLeft="24dp"
android:layout_marginTop="8dp"
android:layout_marginTop="20dp"
Copy link
Member

Choose a reason for hiding this comment

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

이거 30dp 같은데 확인 부탁드립니당~

android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="22dp"
android:src="@{viewModel.isValidEmail() == true? @drawable/ic_smile : @drawable/ic_tears}"
Copy link
Member

Choose a reason for hiding this comment

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

good

app:layout_constraintTop_toBottomOf="@id/view_bottom_sheet_top"
tools:src="@drawable/ic_smile" />

<androidx.constraintlayout.widget.ConstraintLayout
Copy link
Member

Choose a reason for hiding this comment

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

이걸 ConstraintLayout으로 감싼 이유는?!

android:layout_marginVertical="48dp"
android:backgroundTint="@color/gray_700"
android:paddingVertical="18dp"
android:text="확인"
Copy link
Member

Choose a reason for hiding this comment

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

string 추출

android:textAppearance="@style/TextAppearance.Headline"
android:textColor="@color/gray_100"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 구현할거면 glStart, glEnd는 안쓰는거 같아서 두 개 삭제 해주세용! 여기에만 쓰여서 guideline을 삭제하는게 맞을 듯!

Comment on lines 61 to 68
private fun checkEmailCondition(): Boolean {
return isValidEmail.value == true
}

private fun checkNicknameCondition(): Boolean {
return isValidNickname.value == true
}

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
Member

Choose a reason for hiding this comment

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

아 메니페스트 추가했는지도 확인부탁!

Copy link
Member

@Dan2dani Dan2dani left a comment

Choose a reason for hiding this comment

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

리뷰 화긴~~

@@ -31,6 +31,22 @@
android:exported="false"
android:screenOrientation="portrait"
tools:ignore="LockedOrientationActivity" />
<activity
Copy link
Member

Choose a reason for hiding this comment

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

exported 전부 false로 바꿔주세요

@@ -31,22 +31,6 @@
android:exported="false"
android:screenOrientation="portrait"
tools:ignore="LockedOrientationActivity" />
<activity
Copy link
Member

Choose a reason for hiding this comment

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

이걸 왜 다 지웟죠?!?!?!

app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintTop_toBottomOf="@+id/linear_nickname" /><!--visibility viewModel 만든 후 구현 예정-->

<com.google.android.material.button.MaterialButton
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
<com.google.android.material.button.MaterialButton
<com.google.android.material.button.MaterialButton
app:rippleColor="@android:color/transparent"

Copy link
Member

Choose a reason for hiding this comment

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

버튼 ripple 혀효과 확인하고 반영 부탁!

class SignUpViewModel @Inject constructor() : ViewModel() {
val email = MutableLiveData("")
val password = MutableLiveData("")
val password_check = MutableLiveData("")
Copy link
Member

Choose a reason for hiding this comment

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

변수명 passwordCheck camelcase 지켜주세요

Comment on lines 72 to 73
const val PASSWORD_PATTERN =
"^(?=.*[A-Za-z])(?=.*[0-9])(?=.*[\$@\$!%*#?&.])[A-Za-z[0-9]\$@\$!%*#?&.]{6,12}\$"
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
const val PASSWORD_PATTERN =
"^(?=.*[A-Za-z])(?=.*[0-9])(?=.*[\$@\$!%*#?&.])[A-Za-z[0-9]\$@\$!%*#?&.]{6,12}\$"
const val PASSWORD_PATTERN = "^(?=.*[A-Za-z\\d])[A-Za-z\\d]{8,}$"

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
Member

Choose a reason for hiding this comment

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

나머지 리뷰는 내일 달겠습니다 ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 파업이슈

@jooyyoo jooyyoo changed the title [feat] signup 뷰 구현 [feat] 회원가입 뷰 구현 Jul 15, 2023
@jooyyoo jooyyoo merged commit d7e0e03 into develop Jul 17, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[feat] 회원가입 뷰 구현
3 participants