Skip to content

Step2 - 페이먼츠(카드 목록) #31

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

Merged
merged 28 commits into from
Aug 26, 2024
Merged

Conversation

oyj7677
Copy link

@oyj7677 oyj7677 commented Aug 25, 2024

compose에서 사용되는 viewModel과 flow가 아직 익숙하지 않아 시간이 걸렸습니다.

CardListViewModel의 fetchCards()를 init()에서 실행 시켜주어야만 카드 등록 후 카드 목록으로 돌아왔을 때 리컴포지션이 발생하는 걸로 확인됩니다. (다른분의 코드를 참조하였습니다.)

이해가 아직 잘 안되는데 이 부분에 관련해서 무엇을 학습해야하는지 알려주시면 감사드리겠습니다.

감사합니다.

oyj7677 added 17 commits August 23, 2024 14:20
1. Card 데이터 클래스 생성
2. 카드 저장소 오브젝트 생성
3. NewCardViewModel 파라미터로 카드 저장소 오브젝트 추가(디폴트)
1. 카드 등록 엑티비티 생성
2. 카드 리스트 스크린 생성
3. MainActivity 화면 로딩 변경 (NewCardScreen -> CardListScreen)
1. CardListTopBar 컴포넌트 생성
2. CardListScreen의 Scaffold(topBar) 에 적용
3. LazyColumn 세팅
1. 카드 번호, 사용자명, 유효기간 노출
2. 카드번호 마스킹 미구현
3. IC 노출
1. 등록된 카드 이미지 - ui.card.list.component.card
1. 카드 등록 이미지 - EmptyCardImage
2. 카드 등록 코멘트 - CardRegistrationComment
1. 카드 등록이 안되있다면 카드 등록 코멘트 노출
2. 등록된 카드 리스트 노출
3. 카드 등록 이미지 노출
4. 테스트 태그 적용
1. 빈 카드 이미지 클릭 시 카드 등록 엑티비티 이동
1. 새로운 카드가 추가되었을 때 카드 목록이 업데이트 되어야 한다.
1. CardListViewModel 생성
2. CardList 일급객체 생성
3. 카드 리스트 LazyColum Item 변경 (PaymentCardsRepository.cards -> CardList)
4. 빈 카드 항목 클릭 상태 호이스팅 적용
5. newCardScreen 탑바 버튼 상태 호이스팅 적용
1. 등록된 카드 UI 상태값 추가 (CreditCardUiState)
2. 카드 상태 반환 함수 구현
3. 카드 UI상태값에 따른 화면 추가
1. _Credit_card -> _creditCard
Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

안녕하세요 영재님!

3단계 구현을 잘 해주셨습니다 💯 가능한 요구 사항을 최대한 테스트하고 Preview하도록 꼼꼼히 구현하신 것이 인상적이였습니다.
고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏 💪

) {
if (cards.cardList.isEmpty()) {
CardRegistrationComment(
comment = "새로운 카드를 등록해주세요.",
Copy link
Member

Choose a reason for hiding this comment

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

정적 문자열 리소스를 노출할 때에는 stringResource를 활용해보면 어떨까요~? 전체 파일에 적용해주세요!

Copy link
Member

@wisemuji wisemuji Aug 25, 2024

Choose a reason for hiding this comment

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

또한 CardRegistrationComment를 사용하는 곳에서 모두 동일한 문자열을 넣어주고 있어요.

컴포넌트를 나누는 기준은 개발자마다 제각각인데요,
이 외에도 다소의 컴포넌트를 별도의 파일로 분리하여 최대한 하나의 역할만 할 수 있도록 구현해주셨지만, 동시에 외부에 너무 많은 컴포넌트가 공개되어 어쩌면 캡슐화 가능한 컴포넌트도 드러나게 됩니다.(예를 들어 CardRegistrationComment은 나중에 재사용될 여지가 있을까 🤔 , 재사용 가능하지 않다면 무조건 캡슐화해야 하나?)

이번 기회에 본인만의 기준을 새우는 과정이 되었으면 합니다!
다음 글이 도움되면 좋겠어요!
https://thdev.tech/compose/2024/08/04/Android-Compose-Split-Funcation

Copy link
Author

@oyj7677 oyj7677 Aug 26, 2024

Choose a reason for hiding this comment

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

해당 Text가 어떤 의도를 가지고 있는지 표시함과 동시에 코멘트가 수정되어 사용될 수 있겠다 싶어서 분리를 했었습니다.
첨부해주신 글을 읽고나니 파라미터로 코멘트할 내용을 받아서 Text 컴포넌트로 사용하여도 충분히 의도를 설명할 수 있다고 판단하였습니다.
현재 작은 프로젝트지만 컴포넌트가 많아 관리가 어려울때가 있었는데 컴포넌트 분리의 기준을 새우는데 많은 도움이 되었습니다.
감사합니다.

ccb10ab d7cf0c0

Comment on lines 111 to 120
if (cards.cardList.isEmpty()) {
CardRegistrationComment(
comment = "새로운 카드를 등록해주세요.",
modifier = Modifier
.fillMaxWidth()
.padding(horizontal = 73.dp)
)
} else {
CardLazyColumn(cards)
}
Copy link
Member

Choose a reason for hiding this comment

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

CardListScreenOne 컴포넌트를 노출한다는 것 자체가 카드 리스트에 카드가 하나만 존재한다는 의미이므로 이 조건문은 무의미할 것 같아요.

CardListScreenOne의 파라미터로 카드 하나만 받도록 설계해볼 수 있지 않을까요?

Copy link
Author

@oyj7677 oyj7677 Aug 26, 2024

Choose a reason for hiding this comment

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

UI상태값을 적용하는 과정에서 복붙을 하다보니 조건문이 중복되어버렸네요.
해당 부분 수정했습니다.
6f7fbd9

fun CardListScreenOne(
cards: CreditCard,
onAddCard: () -> Unit = {},
modifier: Modifier = Modifier
Copy link
Member

Choose a reason for hiding this comment

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

modifier를 넘기고 있지만 사용되고 있지 않아요!

Copy link
Author

@oyj7677 oyj7677 Aug 26, 2024

Choose a reason for hiding this comment

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

나중에 사용할지도 모른다는 생각에 남겨놨었는데 당장 불필요한 파라미터는 제거하겠습니다.
해당 부분 수정했습니다.
6f7fbd9

import nextstep.payments.ui.theme.PaymentsTheme

@Composable
fun CardImage(card: Card, cardColor: Color, modifier: Modifier = Modifier) {
Copy link
Member

Choose a reason for hiding this comment

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

새로운 CardImage를 만들기보다 기존에 잘 만들어놓은 PaymentCard를 재사용하면 어떨까요? 🙂
디자인 가이드를 보시면 카드 정보가 추가된 것을 제외하고는 모두 동일합니다.

Copy link
Author

Choose a reason for hiding this comment

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

엇.. 생각하지 못했었네요.
PayemntCard를 재사용하지는 안았고 오버로딩을 통해서 구현했습니다.
card 데이터를 디폴드 파라미터를 설정해주는 것보다 따로 만드는게 더 깔끔해 보인다고 생각했습니다.
오버로딩도 재사용인지는 잘 모르겠습니다.
89d2f55 - 오버로딩 구현 및 컴포넌트 적용
2840f8e - CardImage 컴포넌트 삭제

@@ -78,32 +111,32 @@ private fun NewCardScreen(
OutlinedTextField(
value = cardNumber,
onValueChange = { setCardNumber(it) },
label = { Text("카드 번호") },
placeholder = { Text("0000 - 0000 - 0000 - 0000") },
label = { stringResource(id = R.string.text_card_number) },
Copy link
Member

Choose a reason for hiding this comment

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

Text로 감싸줘야 실제로 UI 상 가시적으로 그려집니다! 🙂

Suggested change
label = { stringResource(id = R.string.text_card_number) },
label = { Text(stringResource(id = R.string.text_card_number)) },

Copy link
Author

Choose a reason for hiding this comment

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

어쩐지... UI가 예상과 다르게 적용됐는데... Text로 감싸지 않아서 그랬던 거군요.
d20d0e2

Comment on lines 171 to 192
@Preview
@Composable
private fun CardListScreenOnePreview() {
PaymentCardsRepository.addCard(
Card(
cardNumber = "1234-5678-1234-5678",
ownerName = "홍길동",
expiredDate = "12/24",
password = "123",
cardCompany = BcCard
)
)

val viewModel = CardListViewModel()

PaymentsTheme {
CardListScreen(
viewModel = viewModel,
onAddCard = {}
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Preview에서는 PaymentCardsRepository를 조작하는 등 이러한 직접적인 비즈니스 로직을 다루지 않아야 합니다.

이렇기 때문에 ViewModel을 파라미터로 받는 형태의 컴포저블을 직접 테스트하기보단(혹은 Preview), 상태 호이스팅을 통하여 cards를 파라미터로 받는 컴포저블을 만들어보면 어떨까요? 상태를 들고 있는 컴포저블과 UI를 노출하는 컴포저블을 분리하여 UI를 노출하는 컴포저블에 대해서만 Preview를 작성하는 겁니다.

그 외에 다음과 같은 방법도 있으니 참고만 해주세요!
https://medium.com/@skydoves/designing-effective-uis-for-enhancing-compose-previews-daa8565e109f

Copy link
Author

Choose a reason for hiding this comment

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

card데이터를 파라미터로 받는 컴포저블을 만들어 개선하였습니다.
preview에서 viewModel이 사용되었을 때보다 코드가 간결해지고 PaymentCardsRepository 오브젝트에 저장되지 않으니 LazyColumn의 key값이 중복됐던 이슈도 발생할 일이 업어졌네요.
70b7d52

val creditCard: StateFlow<CreditCard> = _creditCard.asStateFlow()

init {
fetchCards()
Copy link
Member

Choose a reason for hiding this comment

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

CardListViewModel의 fetchCards()를 init()에서 실행 시켜주어야만 카드 등록 후 카드 목록으로 돌아왔을 때 리컴포지션이 발생하는 걸로 확인됩니다.

제가 테스트했을 때에는 init 블럭 제거하고도 rememberLauncherForActivityResult를 통해 잘 로딩되는 것으로 보이는데요, 혹시 어떻게 테스트하셨던걸까요~?

Copy link
Author

Choose a reason for hiding this comment

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

다시 테스트 해보니 init() 블럭을 제거해도 정상적으로 로딩되는 것 확인했습니다.
init()에서 fetchCards()를 실행시키는 문제가 아니었나 봅니다.
이슈가 생기면 자세히 기록해두는 습관을 길러야겠네요 ㅜㅜ
감사합니다.

Comment on lines +14 to +21
@Composable
fun CardNumber(cardNumber: String, modifier: Modifier = Modifier) {
val numberList = cardNumber.split("-")

Row(
modifier = modifier.fillMaxWidth(),
horizontalArrangement = Arrangement.SpaceBetween
) {
Copy link
Member

Choose a reason for hiding this comment

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

하나의 Text로 구현하지 않으신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

하나의 Text로는 4자리 숫자와 '-' 을 적절한 간격으로 구현하기 어렵다고 판단하였습니다.
카드번호에 대한 유효성 검사 로직은 없지만, 고정적으로 숫자 16개와 3개의 '-'(하이픈)으로 구성되어야 하기 때문에 현재의 컴포넌트 구성으로 작성하게 되었습니다.
하나의 Text를 사용하려면 어떤 식으로 구성해야 하나요?

Copy link
Member

Choose a reason for hiding this comment

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

디자인 가이드에 따라 자간을 조정하시면 원하는 바를 얻으실 수 있을거예요!
중요한 학습 목표는 아니니 반영 여부는 영재님이 선택해주세요 🙂


@Composable
fun EmptyCardImage(cardColor: Color, onAddCard: () -> Unit, modifier: Modifier = Modifier) {
val context = LocalContext.current
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
Author

Choose a reason for hiding this comment

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

코드를 꼼꼼히 보겠습니다. ㅜㅜ

1f21996

import nextstep.payments.ui.theme.PaymentsTheme

@Composable
fun EmptyCardImage(cardColor: Color, onAddCard: () -> Unit, modifier: Modifier = Modifier) {
Copy link
Member

Choose a reason for hiding this comment

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

클릭 영역이 둥근 모서리로 클립되도록 바꿔보면 어떨까요?
image

Copy link
Author

Choose a reason for hiding this comment

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

Card의 clip 속성을 사용하여 수정하였습니다.
Card 컴포넌트에 특별한 모양을 부여하지 않아 기본 값인 CardDefaults.shape을 사용하였습니다.
ebaac53

oyj7677 added 11 commits August 26, 2024 09:13
1. 사용하지 않는 modifier 및 card 데이터 제거
2. UI상태값 활용으로 인한 UI 노출 로직 제거.
1. 사용하려는 리소스를 Text로 감싸주어 해결
2. modifier 파라미터 순서 변경
1. CreditCradUiState 패키지 변경
 data ->  ui.card
2. CreditCards(val cardList : List<Card>) -> RegisteredCreditCards(val cardList : List<Card>)
1. 비즈니스 로직이 들어가지 않는 CardListScreen 컴포넌트 추가
2. 파라미터 명 변경 (cards -> registeredCreditCards), cards는 카드 리스트를 의미함
1. clip(shape = CardDefaults.shape) 속성을 추가하여 둥근 모서리에 맞게 수정
1. CardRegistrationComment 대신 Text 컴포넌트 사용.
2. 표시할 코멘트를 파라미터로 받아서 사용.
1. 해당 로직을 사용하지 않아도 정상적으로 UI가 구현됨
1. 등록된 카드 항목 이미지에 사용
1. PaymentCard 오버로딩으로 대체
Copy link
Member

@wisemuji wisemuji 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 +14 to +21
@Composable
fun CardNumber(cardNumber: String, modifier: Modifier = Modifier) {
val numberList = cardNumber.split("-")

Row(
modifier = modifier.fillMaxWidth(),
horizontalArrangement = Arrangement.SpaceBetween
) {
Copy link
Member

Choose a reason for hiding this comment

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

디자인 가이드에 따라 자간을 조정하시면 원하는 바를 얻으실 수 있을거예요!
중요한 학습 목표는 아니니 반영 여부는 영재님이 선택해주세요 🙂

Comment on lines 3 to 12
data class CreditCard(val cardList: List<Card>) {

fun getState(): CreditCardUiState {
return when (cardList.size) {
0 -> CreditCardUiState.Empty
1 -> CreditCardUiState.One(cardList[0])
else -> CreditCardUiState.Many(cardList)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

일반적으로 안드로이드 권장 아키텍처에서는 UI를 어떻게 노출할까?에 대한 관심사를 UI Layer에 두곤 하는데요,
(예를 들면 카드가 비었을 때에는 특정 문구를 노출한다던지)

4주차 미션을 진행하시면서 자연스럽게 학습하게 되실 내용이라 여기서는 자세히 다루진 않을게요!

Comment on lines +50 to +66
@Composable
fun PaymentCard(
card: Card,
cardColor: Color,
modifier: Modifier = Modifier,
) {
Box(
contentAlignment = Alignment.BottomEnd,
modifier = modifier
.shadow(8.dp)
.size(width = 208.dp, height = 124.dp)
.background(
color = cardColor,
shape = RoundedCornerShape(5.dp),
)
.padding(bottom = 10.dp)
) {
Copy link
Member

Choose a reason for hiding this comment

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

예를 들어 원본 PaymentCard에 @Composable () -> Unit 타입을 받아서 이러한 로직을 재사용해볼 수 있지 않을까요? 피드백 강의에서 자세히 다루니 참고해주세요!

Copy link
Author

Choose a reason for hiding this comment

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

말씀해주신 것 적용했습니다. 아직 익숙하지 않아 적절하게 구성했는지는 모르겠네요.
특히, 카드 이미지 컴포넌트를 재사용하여 만들어진 컴포넌트를 구현할 때 해당 컴포넌트 단일로는 정상적으로 preview가 나왔지만 LazyColumn에 적용됐을 때는 재사용하는 컴포넌트(기존 PaymentCard)와 추가된 컴포넌트가(카드 번호, 유효 기간, 이름) column의 구성처럼 영역을 잡고 있었습니다.
아래 사진과 같았습니다.

스크린샷 2024-08-27 오후 2 58 45

혹 어떤 컴포넌트의 속성 때문에 그런 것인지 알 수 있을까요?

우선 이를 해결하기 위해 새로운 컴포넌트에 재사용하는 컴포넌트와 추가된 컴포넌트를 Box로 감싸주었습니다.

115f298

@Preview
@Composable
private fun CardListScreenEmptyPreview() {
PaymentCardsRepository.removeAllCard()
Copy link
Member

Choose a reason for hiding this comment

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

상태 호이스팅을 활용해서 구조를 잘 설계해주시고 Preview를 쉽게 잘 구현해주셨어요!
이제 PaymentCardsRepository 코드는 다 제거할 수 있겠네요!

Copy link
Author

Choose a reason for hiding this comment

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

꼼꼼하지 못했네요 ㅜㅜ 수정했습니다.

66f86fe

@wisemuji wisemuji merged commit 2d0e753 into next-step:oyj7677 Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants