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

[PC-257] 접근 권한, 지인 피하기, 회원가입 완료 UI 구현 #29

Merged
merged 16 commits into from
Jan 11, 2025

Conversation

tgyuuAn
Copy link
Member

@tgyuuAn tgyuuAn commented Jan 9, 2025

PC-257

1. ⭐️ 변경된 내용

  • 접근 권한, 지인 피하기, 회원가입 완료 UI 구현
  • Body -> Page 네이밍 변경

2. 🖼️ 스크린샷(선택)

2025-01-10.12.34.11.mov

@tgyuuAn tgyuuAn added UI/UX 🎨 디자인 시스템, 디자인 리소스 관련 코드 🎨 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 ㅌㄱ태규 ☀️ 훗날 크게될 ENFP 남성, tgyuuAn labels Jan 9, 2025
@tgyuuAn tgyuuAn requested a review from sksowk156 January 9, 2025 15:39
@tgyuuAn tgyuuAn self-assigned this Jan 9, 2025
Copy link
Contributor

@sksowk156 sksowk156 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 +69 to +70
setSelectedTerm(it)
onTermDetailClick()
Copy link
Contributor

Choose a reason for hiding this comment

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

-p4 : 혹시 onTermDetailClick과 setSelectedTerm를 분리해서 하신 이유가 있으신가요? selectedTerm을 SignUpState에서 관리하면 onTermDetailClick(it)로 페이지 정보랑 클릭 이벤트가 하나의 메서드로 관리할 수 있을 것 같은데 혹시 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

-p4 : 혹시 onTermDetailClick과 setSelectedTerm를 분리해서 하신 이유가 있으신가요? selectedTerm을 SignUpState에서 관리하면 onTermDetailClick(it)로 페이지 정보랑 클릭 이벤트가 하나의 메서드로 관리할 수 있을 것 같은데 혹시 어떻게 생각하시나요?

seletedTerm을 ViewModel으로 올려도 될 것 같지 않아서 이렇게 구현했습니다!

상태 호이스팅UI 상태는 UI 상태를 읽고 쓰는 모든 컴포저블의 가장 낮은 공통 상위 요소로 호이스팅해야 합니다. 라는 구문이 있기 때문에 이렇게 구현하였습니다!

Copy link
Contributor

@sksowk156 sksowk156 Jan 11, 2025

Choose a reason for hiding this comment

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

아하 'UI 상태는 UI 상태를 읽고 쓰는 모든 컴포저블의 가장 낮은 공통 상위 요소로 호이스팅해야 합니다.'라는건 selectedTerm 정보가 TermDetailPage와 TermPage에서만 쓰이기에 viewmodel까지 쓰지 않았다는 거군요. 좋은 것 같아요!!

그런데 질문이 하나 있어요! 그럼 TermDetailPage 에서 앱을 회전하거나 하면 NPE가 발생하진 않나요?
TermPage의 showTermDetail 에서 selectedTerm에 값을 할당해서 TermDetailPage 로 넘어간 상태에서, 화면을 회전하는 등 Configuration Change가 발생하면 SignUpState는 TermDetailPage 로 유지되지만 selectedTerm는 유지가 안되서 NPE가 발생할 것 같아서요! 만약 그렇다면 rememberSavable인가? 그게 필요할 것 같아요!

Copy link
Member Author

@tgyuuAn tgyuuAn Jan 11, 2025

Choose a reason for hiding this comment

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

아하 'UI 상태는 UI 상태를 읽고 쓰는 모든 컴포저블의 가장 낮은 공통 상위 요소로 호이스팅해야 합니다.'라는건 selectedTerm 정보가 TermDetailPage와 TermPage에서만 쓰이기에 viewmodel까지 쓰지 않았다는 거군요. 좋은 것 같아요!!

그런데 질문이 하나 있어요! 그럼 TermDetailPage 에서 앱을 회전하거나 하면 NPE가 발생하진 않나요? TermPage의 showTermDetail 에서 selectedTerm에 값을 할당해서 TermDetailPage 로 넘어간 상태에서, 화면을 회전하는 등 Configuration Change가 발생하면 SignUpState는 TermDetailPage 로 유지되지만 selectedTerm는 유지가 안되서 NPE가 발생할 것 같아서요! 만약 그렇다면 rememberSavable인가? 그게 필요할 것 같아요!

오 좋아요!!!!

근데 제 생각에 가로모드 UI가 나오지 않는다면 가로모드를 막는 것이 좋을 것 같아요!!!

Manifest에 가로모드 불가능하도록 막는 코드를 추가하는 것은 어떨까요 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

그렇네요 좋아요! 근데 configuration change에 화면 회전말고도 여러 항목이 있으니 selectedTerm 를 rememberSavable로 하면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇네요 좋아요! 근데 configuration change에 화면 회전말고도 여러 항목이 있으니 selectedTerm 를 rememberSavable로 하면 좋을 것 같아요!

수정했습니다~~

Comment on lines 76 to 79
SignUpState.SignUpPage.TermDetailPage -> TermDetailPage(
term = selectedTerm!!,
onBackClick = onBackClick,
onAgreeClick = { agreeTerm(selectedTerm.id) },
Copy link
Contributor

Choose a reason for hiding this comment

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

  • p4 : agreeTerm(selectedTerm.id) 여기서 selectedTerm.id를 입력하는 것을 TermDetailPage 내부에서 처리하고 onAgreeClick을 (Int) -> Unit으로 하는 건 어떻게 생각하시나요? TermDetailPage 에 term이 입력되어 있기 때문에 termid 정보를 내부에서 처리하면 좋을 것 같아서요!

Copy link
Member Author

Choose a reason for hiding this comment

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

  • p4 : agreeTerm(selectedTerm.id) 여기서 selectedTerm.id를 입력하는 것을 TermDetailPage 내부에서 처리하고 onAgreeClick을 (Int) -> Unit으로 하는 건 어떻게 생각하시나요? TermDetailPage 에 term이 입력되어 있기 때문에 termid 정보를 내부에서 처리하면 좋을 것 같아서요!

오 좋은 것 같아요!!

import com.puzzle.designsystem.foundation.PieceTheme

@Composable
internal fun ColumnScope.AvoidAcquaintancesPage(
Copy link
Contributor

Choose a reason for hiding this comment

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

ColumnScope의 확장 함수로 만들면 내부에 column 선언없이도 바깥 column을 적용할 수 있는건가요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

ColumnScope의 확장 함수로 만들면 내부에 column 선언없이도 바깥 column을 적용할 수 있는건가요?!

네 맞습니다!!

)

Text(
text = "연락처에 등록된 번호로 가입한 사용자는\n매칭 대상에서 제외되어, 개인정보가 보호됩니다.",
Copy link
Contributor

Choose a reason for hiding this comment

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

  • p4: stringResource...!!

Copy link
Member Author

Choose a reason for hiding this comment

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

  • p4: stringResource...!!

반영하겠습니다!!

Comment on lines 73 to 74
terms.forEach { term ->
PieceCheckList(
Copy link
Contributor

Choose a reason for hiding this comment

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

  • p4: PieceCheckList 네이밍이 뭔가 여러 개의 check 리스트가 있을 것 같은 느낌인데, 코드는 하나의 체크만 있는 것 같은데 맞나요?

Copy link
Member Author

@tgyuuAn tgyuuAn Jan 11, 2025

Choose a reason for hiding this comment

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

image

피그마에 위와 같이 정의되어 있긴한데, checkRow로 변경하겠습니다!!

val termsCheckedInfo: Map<Int, Boolean> = emptyMap(),
val signUpPage: SignUpPage = SignUpPage.TermPage,
) : MavericksState {
val allTermsAgreed = terms.all { termsCheckedInfo.getOrDefault(it.id, false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

  • p4: 반환 값이 boolean인가요? 저는 boolean이면 항상 is나 has 등으로 시작하는데 그건 어떻게 생각하시나요?
  • p4:
    val allTermsAgreed : Boolen get() = terms.all { termsCheckedInfo.getOrDefault(it.id, false) }

는 어떠신가요? 뭐 어차피 copy를 통해서 termsCheckedInfo가 갱신되기 때문에 allTermsAgreed도 자동으로 갱신될 거라고 생각하지만... 혹시라도 copy를 통해 termsCheckedInfo가 변경되기 전에 termsCheckedInfo가 편집되고 allTermsAgreed 를 쓸 경우에 변경된 termsCheckedInfo으로 적용되지 않을 거 같아서요...!!

Copy link
Member Author

Choose a reason for hiding this comment

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

  • p4:
    val allTermsAgreed : Boolen get() = terms.all { termsCheckedInfo.getOrDefault(it.id, false) }

는 어떠신가요? 뭐 어차피 copy를 통해서 termsCheckedInfo가 갱신되기 때문에 allTermsAgreed도 자동으로 갱신될 거라고 생각하지만... 혹시라도 copy를 통해 termsCheckedInfo가 변경되기 전에 termsCheckedInfo가 편집되고 allTermsAgreed 를 쓸 경우에 변경된 termsCheckedInfo으로 적용되지 않을 거 같아서요...!!

근데 제가 제대로 이해한 게 맞는 지는 모르겠는데,

만약 termsCheckedInfo가 반영될 때 allTermsAgreed가 반영되지 않는 것을 걱정하시는 거라면,

이 부분은 Mavericks에 있는 파생 속성에 관한 내용이라서 괜찮을 것 같아요. https://airbnb.io/mavericks/#/tips




val 프로퍼티를 썼을 때 내부적으로 get()만 사용하게 되는데,

get()을 오버라이딩하면 해당 프로퍼티를 사용할 때 마다 해당 함수를 호출하게 되거든요,

근데 지금 작성된 코드랑은 기능상 동일할 것으로 예측되는데 아닐까요 ?!

이거 관련해서 제가 작성한 글이 있는데.. 제가 잘못알고있는 건지 궁금합니다!

백킹 프로퍼티.. 잘 사용하고 계신가요? 프로퍼티를 알아보고 완벽히 이해 해보아요...! (Kotlin)

Copy link
Member Author

Choose a reason for hiding this comment

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

  • p4: 반환 값이 boolean인가요? 저는 boolean이면 항상 is나 has 등으로 시작하는데 그건 어떻게 생각하시나요?

isAllTermsAgreed 어떤가요 ?!

Copy link
Contributor

Choose a reason for hiding this comment

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

  • 앗 이건 제가 잘못 생각했던 거 같습니다. 태규님 말씀이 맞아요! 허허 죄송합니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

넵넵! areAllTermsAgreed 나 haveAllTermsAgreed면 좋을 것 같습니다!

@sksowk156 sksowk156 added 머지 해도될듯염🌟 현재 코드를 기존 코드에 합쳐도 될 것 같다라고 판단..! 🌟 and removed 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 labels Jan 11, 2025
@tgyuuAn tgyuuAn merged commit f842bfa into develop Jan 11, 2025
1 check passed
@tgyuuAn tgyuuAn deleted the feature/tgyuu/PC-257 branch January 11, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI/UX 🎨 디자인 시스템, 디자인 리소스 관련 코드 🎨 ㅌㄱ태규 ☀️ 훗날 크게될 ENFP 남성, tgyuuAn 머지 해도될듯염🌟 현재 코드를 기존 코드에 합쳐도 될 것 같다라고 판단..! 🌟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants