[악어] 장바구니 미션 제출합니다. #6

Hogu59 · 2024-11-10T11:16:11Z

3단계까지 구현했습니다!

컴포즈를 학습해보고는 있는데 쉽지 않네요...
제임스의 예상 답변은 "저도 몰라요" 겠지만, 그렇다면 같이 학습해보시죠!
(예상답변: "굳이요?" 에 대한 예상 반박으로 "넵" 을 미리 전달드립니다)

질문이 있습니다.
마지막 화면에서 변동사항을 반영하기 위해 cartItems = Cart.items 와 같은 코드를 반복하고 있는데..
이렇게 안 하려면 도대체 어떻게 해야할지... 아직 잘 모르겠습니다...
혹시라도 힌트라도 주신다면.... 감사하겠습니다! 🙏

class CartActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            var cartItems by remember { mutableStateOf(Cart.items) }
            AndroidshoppingcartTheme {
                Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
                    CartLayout(
                        navigation = { finish() },
                        modifier = Modifier.padding(innerPadding),
                        cartItems = cartItems,
                        onIncrease = {
                            onIncrease(it)
                            cartItems = Cart.items
                        },
                        onDecrease = {
                            onDecrease(it)
                            cartItems = Cart.items
                        },
                        onClear = {
                            onRemove(it)
                            cartItems = Cart.items
                        }
                    )
                }
            }
        }
    }

Screen_recording_20241110_200248.mp4

woowajames

악어 안녕하세요.
선택 미션까지 진행하시다니 멋지네요.
몇 가지 코멘트를 남겼으니 한번 확인해 주세요.
테스트 코드를 작성하지 않았던데 3단계 프로그래밍 요구 사항이니 도전해 보세요!

woowajames · 2024-11-11T16:53:59Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartLayout.kt

+    modifier: Modifier = Modifier
+) {
+    Column {
+        Surface {


Scaffold 내부에서 Surface를 다시 정의할 필요는 없습니다.
Scaffold의 내부를 살펴보시면 이미 Surface를 사용하고 있다는 것을 알 수 있습니다.

오 불필요한 모습으로 화면을 구성하고 있었네요!
알려주셔서 감사합니다!

woowajames · 2024-11-11T17:02:57Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartLayout.kt

+    onIncrease: (CartItem) -> Unit = {},
+    onDecrease: (CartItem) -> Unit = {},
+    onClear: (CartItem) -> Unit = {},
+    modifier: Modifier = Modifier


Modifier 타입의 파라미터는 첫 번째 옵셔널 파라미터로 사용하는 것을 권장합니다.

참고: API Guidelines for Jetpack Compose

근데 왜 옵셔널 파라미터로 사용해야 할까요? 🤔

Modifier 타입의 파라미터는 첫 번째 옵셔널 파라미터로 사용하는 것을 권장합니다.

해당 내용 수정했습니다!
알려주셔서 감사합니다!

근데 왜 옵셔널 파라미터로 사용해야 할까요? 🤔

제임스가 공유해준 참고 자료를 읽어보았는데, 제가 아직 문맥을 정확히는 이해하지 못했습니다.
다만, 옵셔널로 주는 이유는 최소의 크기를 가지는 컴포넌트의 경우 modifier를 상위에서 생략할 수 있기 때문에 그런 것이 아닐까 싶습니다.
이외에도, 해당 컴포넌트를 사용하는 곳(상위라는 표현을 저는 섞어서 사용합니다)에서 modifier 값을 선언하든 선언하지 않든 사용할 수 어떻게 보여질 것인지에 대해서만 집중할 수 있도록 하기 위함이 아닐까 싶었습니다.
(상위에서 패딩을 주든 주지 않든 관계 없이 해당 컴포넌트는 어떻게 내부 컴포넌트를 배치할지 혹은 보여줄지에 대해서만 고민할 수 있게 하는 방식을 따를 수 있기 때문에 그런것이 아닐까.... 조심스럽게 추측해봅니다..)

woowajames · 2024-11-11T17:06:51Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

+import nextstep.signup.R
+
+@Composable
+fun ShoppingCartScreen(


컨벤션에 따라 다르겠지만 보통 xxxScreen으로 작성하는 추세인 것 같은데 잘 적용해 주셨네요.
파일명과 다른 이유는 혹시 무엇인가요?

컴포저블 함수가 아직 익숙치 않아서 나온 실수입니다...ㅎㅎ 😅
사실 아직 클래스가 아닌 함수만 들고 있는 파일을 만든다는 것에서부터 이질감을 꽤 느끼고 있는데, 이로부터 비롯된 실수인것 같습니다.

알려주셔서 감사합니다! 수정하였습니다!

woowajames · 2024-11-11T17:07:10Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

+) {
+    val totalPrice = items.sumOf { it.product.price * it.count }
+
+    Scaffold(


이 부분도 불필요합니다. 관련된 부분은 모두 확인해 주세요.

알려주셔서 감사합니다! 전반적으로 검토하고 다시 구성하였습니다! 🫡

woowajames · 2024-11-11T17:07:33Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

+    onIncrease: (CartItem) -> Unit = {},
+    onDecrease: (CartItem) -> Unit = {},
+    onClear: (CartItem) -> Unit = {},
+    modifier: Modifier = Modifier


이 부분 역시 첫 번째 옵셔널 파라미터가 아니네요. 관련된 부분을 모두 점검해 보면 좋을 것 같아요.

위와 동일하게 전반적으로 검토하고 수정해보았습니다! 알려주셔서 감사합니다!

woowajames · 2024-11-11T17:54:19Z

app/src/main/java/nextstep/shoppingcart/presentation/CartActivity.kt

+                Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
+                    CartLayout(
+                        navigation = { finish() },
+                        modifier = Modifier.padding(innerPadding),
+                        cartItems = cartItems,
+                        onIncrease = {
+                            onIncrease(it)
+                            cartItems = Cart.items
+                        },
+                        onDecrease = {
+                            onDecrease(it)
+                            cartItems = Cart.items
+                        },
+                        onClear = {
+                            onRemove(it)
+                            cartItems = Cart.items
+                        }
+                    )
+                }


onIncrease() 메서드 등에서 모델을 통해 items 값을 변경하도록 잘 해주셨네요.
앞서 배운 내용을 돌이켜 보면 모델을 통해 값(상태)을 변경하면 그 상태를 observe할 수 있는 방법이 있지 않았나요?

레아가 강의 자료에 올려주신 SnapshotStateList 를 활용해서 리팩토링해볼 수 있을 것 같습니다!

이렇게 바로 답을 알려주시다니... 악어가 이것저것 해보고 스스로 찾아가길 의도한 것이었는데... 😢

#6 (comment)

오둥이가 잘 알려줬지만, 애초에 아는바가 적어서 flow 찍고 돌아왔습니다...ㅋㅋㅋ
이런 좋은게 있었다니...
개인적으로 flow를 적용하다보니 코드가 너무 많아지는 것 같아서 만족스럽지 않았는데, 적용해보니 문제가 해결되었습니다! 😄

woowajames · 2024-11-11T17:59:34Z

app/src/main/java/nextstep/shoppingcart/presentation/components/DetailLayout.kt

+        bottomBar = {
+            Box(
+                modifier =
+                Modifier
+                    .fillMaxWidth()
+                    .background(Color(0xFF2196F3))
+                    .padding(16.dp)
+                    .clickable { action() },
+                contentAlignment = Alignment.Center
+            ) {
+                Text(
+                    text = stringResource(R.string.add_to_cart),
+                    fontSize = 20.sp,
+                    style =
+                    TextStyle(
+                        color = Color.White,
+                        fontWeight = FontWeight.Bold
+                    )
+                )
+            }
+        }


ShoppingCartScreen이 bottomBar와 거의 유사합니다.
혹시 컴포저블을 분리해서 재사용을 할 수 있겠다는 생각도 드는데요.
어떤 기준으로 별도로 작성하셨나요?

어떤 기준으로 별도로 작성하셨나요?

사실 기준이 제대로 없었습니다. 놀랍게도 Scaffold에 bottomBar 속성이 있다는 것을 발견하고 바로 만들다보니 그렇게 된 것 같습니다..ㅎㅎ 😅
해당 내용 재사용할 수 있도록 수정했습니다!

woowajames · 2024-11-11T18:03:10Z

app/src/main/java/nextstep/shoppingcart/presentation/components/DetailLayout.kt

+            TopBarWithNavigation(
+                name = stringResource(R.string.title_product_detail),
+                navigation = { navigation() },
+                modifier = modifier
+            )


TopBar는 재사용중이네요. bottomBar와 어떤 차이가 있어서 재사용중일까요?
악어만의 재사용할 때의 기준은 무엇인가요?

컴포즈 설계가 아직 미숙해서 재사용을 생각을 못했습니다!
해당 내용 재사용할 수 있도록 변경해보았습니다! 😄

woowajames · 2024-11-11T18:07:04Z

app/src/main/java/nextstep/shoppingcart/presentation/DetailActivity.kt

+import nextstep.shoppingcart.presentation.components.DetailLayout
+import nextstep.shoppingcart.presentation.ui.theme.AndroidshoppingcartTheme
+
+class DetailActivity : ComponentActivity() {


액티비티와 컴포저블 관련부를 나눠도 좋겠다는 생각이 드는데 악어는 어떻게 생각하세요?
가독성이 좋아지고 UI 관련은 컴포저블 함수를 다루는 클래스에서만 작업하면 편리할 것 같아서요.
데이터바인딩을 사용하면 액티비티에서 뷰 관련 코드가 많이 줄어들기도 하잖아요. 그런 관점에서 뷰(=컴포저블 관련 부분)으로 생각해 볼 수 있지 않을까요?

오 그렇네요! 알려주셔서 감사합니다!
해당내용 반영했습니다!

(컴포저블 관련부를 분리하는 것도 사실상 기본적인 부분이었던 것 같은데,
새로운 것을 배울 때마다 기본적인 부분을 잘 생각하지 못한 것 같습니다...ㅎㅎ 😅 )

woowajames · 2024-11-11T18:17:47Z

app/src/main/java/nextstep/shoppingcart/presentation/components/HomeLayout.kt

+        ProductList(
+            items = products,
+            onItemClick = onItemClick,
+            modifier = Modifier.padding(top = 18.dp, start = 13.dp, end = 13.dp)


개발자마다 의견이 갈릴 수 있는 부분인데 dp 값을 이렇게 매직 넘버로 선언한 이유가 있나요?
xml로 작업할 때는 어떻게 하셨나요? 차이가 있다면 컴포즈에선 왜 다를 게 작성했나요?

(뜨끔..) 😂

제임스께서 이미 개발자마다 의견이 갈릴 수 있는 부분이라고 언급해주셨는데요,
전 개인적으로 이 맥락에서의 dp가 매직 넘버라고 생각하진 않습니다.
현재 코드에서는 named arguments를 통해 해당 숫자의 의미를 충분히 파악할 수 있으며,
UI를 작성할 때에는 오히려 별도의 상수를 분리할 때 가독성이 더 떨어질 수 있다고 생각해요.

https://en.wikipedia.org/wiki/Magic_number_(programming)

In computer programming, a magic number is any of the following:
A unique value with unexplained meaning or multiple occurrences which could (preferably) be replaced with a named constant
...

두가지 의견이 있는데,

저는 컴포저블 함수(이하 컴포넌트)에 대해서는 xml 과 큰 차이가 없다라는 생각을 했던 것 같습니다.

그리고 마땅히 어떻게 나눠야할지도 정확히는 모르겠습니다.

1번에 대해서 말하자면, xml에서 패딩이나 마진 값을 상수화하지 않는 것과 같은 맥락으로 안하는 것이 맞지 않나... 라는 생각이었습니다.
2번에 대해서 말하자면, top = 20.dp를 상수화 한다면,

val topPadding = 20.dp modifier = Modifier.padding(top = topPadding)

와 같은 내용이 될 텐데, 그러면 의미있는 분리가 되는 것이 아니라고 생각했습니다.
분리를 위한 분리가 될 것 같았고, 그렇다면 나중에 제가 제대로 설명하지 못하는 설계가 될 것 같았습니다.

(사실 레아의 댓글이 조금 용기를 내도록 도움이 되었던 것 같은데, 이 부분은 실제로 만들면서도 상수화를 고민하긴 했었는데 위와 같은 생각을 하게 되어서 제임스의 의견을 한번 더 듣고 싶습니다!)

제 의견을 다시 물어 보셨는데, 저도 반응형 UI 지원으로 인해서 dimen으로 특별히 관리할 부분은 많이 줄었다고 생각합니다.

murjune

안녕하세요 ~~객체아저씨~~ 악어~ 3단계 요구사항을 잘 반영해주네요 💯

제임스가 이미 피드백을 잘 남겨주셨지만, 저도 추가적으로 리뷰를 남겨봅니다.
남은 레벨 5도 화이팅입니다요 💪

murjune · 2024-11-12T06:19:25Z

app/src/main/java/nextstep/shoppingcart/data/CachedProductRepository.kt

+
+import nextstep.shoppingcart.domain.model.Product
+
+class CachedProductRepository(


레포지토리 패턴은 '데이터의 출처를 추상화'하는 것입니다. 하지만, 네이밍으로 유추해봤을 때 캐시된 데이터를 가져오는 것을 클라이언트가 확인할 수 있네요.

악어는 Repository 패턴을 사용하시는 다른 이유가 있으신가요?? 🤔

두 가지 이유에서였는데,

Repository를 만드는게 그렇게 어렵지 않다고 생각합니다. (이건 익숙함에서 오는 것이라고 생각합니다)

레포지토리 패턴을 사용하면 데이터소스를 변경하는 것이 용이해진다고 생각해서 이렇게 작성했습니다.
개인적인 생각으로는 이 미션도 장바구니 미션처럼 api를 받아올 것이라고 생각했어서인지, 열어놓고 생각해야한다고 생각했었습니다.
다만, 다시 살펴보니 여기에서는 굳이 Repository에 Cached prefix를 붙이지 않고 Default로 쓰고 레포지토리 상위에서 주입하는 데이터소스를 바꿔주는게 맞을 것 같아서 수정했습니다!

murjune · 2024-11-12T06:21:25Z

app/src/main/java/nextstep/shoppingcart/data/Cart.kt

+import nextstep.shoppingcart.domain.model.CartItem
+import nextstep.shoppingcart.domain.model.Product
+
+object Cart {


Cart 에는 레포지토리 패턴을 적용하지 않으신 이유가 있을까요??

product 때까지만 해도 확장성을 열어놓고 생각했었는데요,
만들고 나니 현재로서는 레포지토리 패턴으로 확장하지 않아도 되었겠다는 생각이 들더라구요.
그래서 주어진 Cart 코드로만 작성해봐야겠다 라는 의도로 적용하지 않았습니다...ㅎㅎ

murjune · 2024-11-12T06:23:33Z

app/src/main/java/nextstep/shoppingcart/domain/model/Product.kt

+    val isAddedToCart: Boolean = false
+) {
+    companion object {
+        val NULL_PRODUCT = Product(-1, "Null Product", -1000000, "", false)


널 프로덕트가 뭔가요?? ㅇㅅㅇ

널 객체처럼...ㅋㅋㅋ
저렇게 하면 compose preview 볼 때 간단하게 값을 넣어서 보기가 편해서 해봤는데...ㅋㅋㅋ
혹시 더 좋은 방법이 있다면 알려주시면 감사하겠습니다!

murjune · 2024-11-12T06:40:56Z

app/src/main/java/nextstep/shoppingcart/presentation/CartActivity.kt

+                Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
+                    CartLayout(
+                        navigation = { finish() },
+                        modifier = Modifier.padding(innerPadding),
+                        cartItems = cartItems,
+                        onIncrease = {
+                            onIncrease(it)
+                            cartItems = Cart.items
+                        },
+                        onDecrease = {
+                            onDecrease(it)
+                            cartItems = Cart.items
+                        },
+                        onClear = {
+                            onRemove(it)
+                            cartItems = Cart.items
+                        }
+                    )
+                }


레아가 강의 자료에 올려주신 SnapshotStateList 를 활용해서 리팩토링해볼 수 있을 것 같습니다!

murjune · 2024-11-12T06:42:09Z

app/src/main/java/nextstep/shoppingcart/presentation/DetailActivity.kt

+        val product = fetchProduct()
+        setContent {
+            AndroidshoppingcartTheme {
+                DetailLayout(
+                    product = product,


요구사항에는 없었지만 만약 product 에 변경사항이 있더라도 Compose 가 감지할 수 없을 것 같아요!

var product by mutableStateOf(fetchProduct())

로 변경했는데, 의도하신 바가 맞는지 궁금합니다...ㅎㅎㅎ 😅

murjune · 2024-11-12T07:02:39Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

+                AsyncImage(
+                    model = item.product.imageUrl,
+                    contentDescription = item.product.name,
+                    modifier =
+                    Modifier
+                        .width(136.dp)
+                        .height(84.dp)
+                )


AsyncImage 를 한 번 wrapping 한 기본 컴포넌트로 분리해보면 어떨까요??

레벨 1때 배웠던 것처럼 서드파티 라이브러리의 경우 Adapter 패턴을 사용하여 한 번 감싸는 것이 좋습니다. 만약, 다른 �라이브러리(Glide)로 대체하는 경우가 생긴다면 더 유연하게 대처할 수 있을 것이에요!

참고로, 얼마전에 Picasso 가 deprecated 됐더라구요.

오... 그렇네요!
현재로서는 라이브러리를 변경하면 존재하는 모든 이미지 코드들을 수정해야했네요...!

의도를 제대로 반영한 것인지는 모르겠지만,
ProductImage라는 컴포넌트로 따로 빼서 변경해보았습니다!

알려주셔서 감사합니다!

murjune · 2024-11-12T07:05:27Z

app/src/main/java/nextstep/shoppingcart/presentation/components/DetailLayout.kt

+import nextstep.signup.R
+
+@Composable
+fun DetailLayout(


DetailLayout 과 ShoppingCartScreen 은 동일하게 '화면'이라는 역할을 가지고 있습니다. 네이밍의 일관성을 지켜보면 어떨까요?🤔

화면(액티비티 전체 화면)은 Screen postfix로 통일시켰습니다!

murjune · 2024-11-12T07:08:09Z

app/src/main/java/nextstep/shoppingcart/presentation/components/DetailLayout.kt

+                    .padding(16.dp)
+                    .clickable { action() },


padding 과 clickable 의 위치가 바뀐 것 같습니다.
padding을 먼저 선언할 경우, padding이 적용된 영역에만 click 영역이 잡히게 됩니다.😉

오... clickable을 먼저 설정하는거군요!
수업시간에 들었던것 같은데 금방 까먹어버렸네요... 😅
덕분에 배웠습니다! 🙇

murjune · 2024-11-12T07:08:50Z

app/src/main/java/nextstep/shoppingcart/presentation/components/HomeLayout.kt

+        ProductList(
+            items = products,
+            onItemClick = onItemClick,
+            modifier = Modifier.padding(top = 18.dp, start = 13.dp, end = 13.dp)


(뜨끔..) 😂

murjune · 2024-11-12T07:10:34Z

app/src/main/java/nextstep/shoppingcart/presentation/components/ProductList.kt

+        columns = GridCells.Fixed(2),
+        modifier = Modifier.padding(12.dp)
+    ) {
+        items(items) { item ->


LazyXXX 관련된 컴포저블을 사용할 때는 key 값을 지정해주는 것이 좋습니다. 😉

관련 공식 문서

오... 처음 알았네요!
알려주셔서 감사합니다! 😄

Hogu59 · 2024-11-19T17:58:13Z

리뷰 반영이 늦어서 죄송합니다....
컴포즈가 처음인데, 제임스 + 오둥이 (feat.레아)의 리뷰에 정신을 못차리고....
매일 열심히 해봤는데 조금 어려워서 늦었습니다.

의견을 남긴 부분도 있고, 아직 의도를 제대로 반영했는지 모호한 부분도 있는데 혹시 괜찮으시다면 확인해주시면 감사하겠습니다...! 🙇

woowajames · 2024-11-23T08:58:02Z

app/src/main/java/nextstep/shoppingcart/presentation/components/screens/CartScreen.kt

    onIncrease: (CartItem) -> Unit = {},
    onDecrease: (CartItem) -> Unit = {},
    onClear: (CartItem) -> Unit = {},
    modifier: Modifier = Modifier,
 ) {
-    val totalPrice = cartItems.sumOf { it.product.price * it.count }
    AndroidshoppingcartTheme {


현재는 큰 문제가 아닐 수 있지만 테마를 Activity에서 지정하지 않은 이유가 있나요?

#6 (comment)

이 코멘트를 반영하려고 모두 Screen 단위 안으로 Theme을 집어넣었습니다.
아마 위쪽 코멘트에서 제가 의도를 잘못 파악한 것 같군요...!

woowajames · 2024-11-23T14:23:42Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

@@ -41,6 +42,7 @@ fun CartItemList(
 ) {
    LazyColumn(
        modifier = modifier,
+        contentPadding = PaddingValues(50.dp)


Scaffold의 innerPadding과는 어떤 차이점이 있나요? (반복 질문이죠?)
원하는 정보를 찾는 것도 개발자에게 중요한 능력 중 하나이지 않을까요?
원하는 정보가 맞춤형으로 존재하지 않는다면 직접 예제를 구현해서 이런저런 실험을 하면서 깨달을 수도 있다고 생각해요.

정확히 모르겠다고 하셨는데, 그럼 어떤 기준으로 이렇게 사용하셨나요?
그리고 디자인 가이드와 너무 많은 차이가 납니다.

woowajames · 2024-11-23T17:32:46Z

app/src/main/java/nextstep/shoppingcart/presentation/components/topbars/TopBarWithNavigation.kt

+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun TopBarWithNavigation(
+    name: String = "상품 상세",


옵셔널 파라미터로 둔 이유가 있나요?
그리고 문자열을 하드 코딩해서 관리하면 어떤 문제점이 있을까요?

woowajames · 2024-11-23T17:34:03Z

app/src/main/java/nextstep/shoppingcart/presentation/components/topbars/TopBarWithAction.kt

+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun TopBarWithAction(
+    name: String = stringResource(R.string.title_product_list),


이 부분도 옵셔널 파라미터일 필요가 있을까요?
가령 실수로 지정하지 않아 예상치 못한 버그가 발생할 가능성이 높아질 것 같습니다.
악어가 옵셔널 파라미터를 사용하는 기준은 무엇일까요?

woowajames · 2024-11-23T17:45:15Z

app/src/main/java/nextstep/shoppingcart/presentation/components/screens/CartScreen.kt

    onIncrease: (CartItem) -> Unit = {},
    onDecrease: (CartItem) -> Unit = {},
    onClear: (CartItem) -> Unit = {},
    modifier: Modifier = Modifier,


이 부분은 여전히 compose api guideline과 일치하지 않네요.

woowajames · 2024-11-23T18:10:14Z

app/src/main/java/nextstep/shoppingcart/presentation/components/BottomBar.kt

+
+@Composable
+fun BottomBar(
+    text: String = stringResource(R.string.add_to_cart),


이 부분도 옵셔널 파라미터가 아니어도 괜찮을 것 같아요.

woowajames · 2024-11-23T18:16:38Z

app/src/main/java/nextstep/shoppingcart/presentation/components/BottomBar.kt

+import nextstep.signup.R
+
+@Composable
+fun BottomBar(


BottomBar를 재사용하기 좋도록 잘 분리하셨네요. 👍

woowajames · 2024-11-23T18:23:07Z

app/src/main/java/nextstep/shoppingcart/presentation/components/CartItemList.kt

@@ -36,76 +31,44 @@ import coil.compose.AsyncImage
 import nextstep.shoppingcart.domain.model.CartItem
 import nextstep.shoppingcart.domain.model.Price


Price는 도메인 모델로 구분하셨네요.
그런데 UI를 위한 포맷팅 작업을 하고 있습니다.
이 작업 UI 관련으로 볼 수도 있는데 도메인 모델에 적절할까요? 참으로 애매한 문제죠? 🤔
참고로 xml을 통해서 해결할 수 있는 방법도 있습니다. ("%,d원")

woowajames · 2024-11-23T18:40:00Z

app/src/main/java/nextstep/shoppingcart/presentation/components/HomeLayout.kt

+        ProductList(
+            items = products,
+            onItemClick = onItemClick,
+            modifier = Modifier.padding(top = 18.dp, start = 13.dp, end = 13.dp)


제 의견을 다시 물어 보셨는데, 저도 반응형 UI 지원으로 인해서 dimen으로 특별히 관리할 부분은 많이 줄었다고 생각합니다.

woowajames

악어! 피드백 잘 반영해 주셨네요.
다만 테스트 코드가 아직 추가되지 않은 점이 아쉽네요.
테스트 코드도 작성해 보면 어떨까요?

woowajames · 2024-11-23T19:00:29Z

app/src/main/java/nextstep/shoppingcart/presentation/components/screens/DetailScreen.kt

@@ -120,5 +112,5 @@ fun DetailLayout(
 @Composable
 @Preview(showBackground = true)
 private fun DetailLayoutPreview() {
-    DetailLayout(product = Product.NULL_PRODUCT)
+    DetailScreen(product = Product.NULL_PRODUCT)


Product에 NULL_PRODUCT이라는 것이 꼭 필요한가요?
외부에서 해당 객체가 어떤 형태인지 파악하기도 어렵지 않나 싶습니다.
또한 프리뷰에서 다양한 테스트를 구성할 수도 있으니 하나로 고정할 필요도 없지 않을까 하는 생각이 들기도 합니다.

woowajames

간단한 코멘트도 남겨봅니다.

Hogu59 added 8 commits November 5, 2024 15:42

style: avoid ktlint to composable functions

71a229c

docs: update README

e4c0cad

feat: implement step1

d417681

docs: update Readme(step2)

1038216

feat: implement step2

29b1d18

docs: update README(step3)

efdf548

feat: implement feature(step3)

bb07f93

fix: change price format

f81c672

woowajames requested changes Nov 11, 2024

View reviewed changes

murjune requested changes Nov 12, 2024

View reviewed changes

Hogu59 added 2 commits November 15, 2024 13:34

refactor(components): optimize component design

959a34d

refactor: apply reviews (murjune)

0882ffd

Hogu59 added 2 commits November 20, 2024 09:36

refactor: apply reviews (james)

f33d230

style: apply ktlint

b03e67d

woowajames reviewed Nov 23, 2024

View reviewed changes

woowajames requested changes Nov 23, 2024

View reviewed changes

woowajames reviewed Nov 23, 2024

View reviewed changes


		import nextstep.shoppingcart.domain.model.Product

		class CachedProductRepository(

		@@ -36,76 +31,44 @@ import coil.compose.AsyncImage
		import nextstep.shoppingcart.domain.model.CartItem
		import nextstep.shoppingcart.domain.model.Price

[악어] 장바구니 미션 제출합니다. #6

Are you sure you want to change the base?

[악어] 장바구니 미션 제출합니다. #6

Conversation

Hogu59 commented Nov 10, 2024

woowajames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hogu59 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hogu59 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

woowajames Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

murjune left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hogu59 commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

woowajames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowajames left a comment

Choose a reason for hiding this comment

Hogu59 Nov 14, 2024 •

edited

Loading

Hogu59 Nov 15, 2024 •

edited

Loading

woowajames Nov 23, 2024 •

edited

Loading

woowajames Nov 23, 2024 •

edited

Loading

woowajames Nov 23, 2024 •

edited

Loading