[Feat/#34] 장소 상세 페이지 StoreInfo, StoreInfoItem 구현 #35

Roel4990 · 2025-01-15T13:24:11Z

Related issue 🛠

closed [FEAT] 장소 상세 페이지 StoreInfo, StoreInfoItem 구현 #34

Work Description ✏️

장소 상세 페이지 StoreInfo, StoreInfoItem 구현

Screenshot 📸

To Reviewers 📢

Hyobeen-Park

코리 끗~~!!

Hyobeen-Park · 2025-01-15T13:45:50Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+            )
+    ) {
+        StoreInfoItem(
+            title = "Menu",


P2: 이 부분 스트링 추출해주면 좋을 것 같네요!

추출완료했습니다!

Hyobeen-Park · 2025-01-15T13:46:09Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+        HorizontalDashedLine()
+        StoreInfoItem(
+            title = "Location",
+            subTitle = "이키",


P1: 이키?!??!?!?!?!?!??!? 잡았다!!

어라라! 수정했습니다! 구조가 많이 바뀌어서 다시한번 확인 부탁드려요!

Hyobeen-Park · 2025-01-15T13:50:22Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+@Composable
+fun StoreInfo(
+    menuItems: List<String>,
+    locationItems: List<String>,


P2: locationItems가 리스트인 이유가 무엇일까요?? 프리뷰를 보니 주소를 의미하는 인자인 것 같은데 주소는 스트링으로 받아도 충분할 것 같아요!

p2) 리스트를 사용하지 말고, immutableList로 추후 수정해주세요

구조가 바뀌어서 수정한 부분 확인 부탁드립니다!

Hyobeen-Park · 2025-01-15T13:50:58Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+import com.spoony.spoony.core.designsystem.theme.SpoonyAndroidTheme
+
+@Composable
+fun StoreInfo(


P1: 상호명이 빠졌어요!! 인자로 추가해주세요~~

구조가 바뀌었습니다! 확인 부탁드려요!

Hyobeen-Park · 2025-01-15T13:52:31Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+fun StoreInfoItem(
+    title: String,
+    subTitle: String? = null,
+    items: List<String>,


P2: 필수인자는 위로!!

수정했습니다!

Hyobeen-Park · 2025-01-15T13:52:55Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+    subTitle: String? = null,
+    items: List<String>,
+    isMenu: Boolean = true,
+    modifier: Modifier = Modifier


P4: modifier는 옵셔널 중 가장 위로 올려주세요😊

수정했습니다!

Hyobeen-Park · 2025-01-15T13:54:40Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+                bottomStart = 8.dp,
+                bottomEnd = 8.dp
+            ) to PaddingValues(
+                vertical = 22.dp,


P1: 패딩값 다시 한번 확인 부탁드려요~!

디쌤분들과 협의하에 수정완료했습니다!

Hyobeen-Park · 2025-01-15T13:57:04Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+            verticalArrangement = Arrangement.spacedBy(12.dp)
+        ) {
+            items.forEach { item ->
+                IconText(


P1: 메뉴일 때랑 상호명일 때 아이콘 색상 달라서 색상도 따로 지정해주세요!

같아보이는데 어떤부분 말씀일까요?!

Hyobeen-Park · 2025-01-15T13:59:26Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+
+@Composable
+private fun IconText(
+    icon: Int,


Suggested change

icon: Int,

@DrawableRes icon: Int,

P3: 필수는 아니지만,, 이렇게 Drawable id만 받을 수 있도록 설정해주는 것도 좋을 것 같아요!

ImageVector로 수정했습니다!

chattymin

고생하셨습니다!
코드리뷰 반영하시고 다시 노티 부탁드립니다!

chattymin · 2025-01-15T14:33:27Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+@Composable
+fun StoreInfo(
+    menuItems: List<String>,
+    locationItems: List<String>,


p2) 리스트를 사용하지 말고, immutableList로 추후 수정해주세요

chattymin · 2025-01-15T14:34:25Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

+    menuItems: List<String>,
+    locationItems: List<String>,
+    modifier: Modifier = Modifier,
+    isBlurred: Boolean = true


p2) isBlurred의 default값이 true인 이유가 있을까요?
서버에서 내려주는 값일텐데 디폴트가 존재하는 이유가 없다고 생각돼요

보이다가 안보이는 것보단 안보이다가 서버에서 isBlurred 가 false 라는것이 떴을 때 보이도록하는게 좋아보였습니다.

default값을 없애는 것이 더 좋아보입니다.

수정완료했습니다.

chattymin · 2025-01-15T15:03:03Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+import com.spoony.spoony.core.designsystem.theme.SpoonyAndroidTheme
+
+@Composable
+fun StoreInfoItem(


p1) StoreInfoItem이라는 컴포넌트를 재활용 하기에 불편한 것 같습니다.
isMenu라는 변수로 두가지 상태를 제어한다는 것이 추후 재사용에 상당히 불리한 것 같습니다.

Base를 하나 만들고 이를 활용한 두개의 컴포넌트를 만드는 방법도 있으니 고민해보시면 좋을 것 같습니다.

구조 수정했습니다 확인 부탁드립니다!

chattymin · 2025-01-15T15:03:44Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+        Column(
+            verticalArrangement = Arrangement.spacedBy(12.dp)
+        ) {
+            items.forEach { item ->


p2) 여기도 key를 추가하면 좋을 것 같습니다.

수정했습니다!

chattymin · 2025-01-15T15:04:56Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

+@Composable
+private fun IconText(
+    icon: Int,
+    iconSize: Int,


p3) size를 나타내려면 dp로 받아오는게 직관적인 것 같아요.

수정했습니다!

chattymin

수정 부탁드립니다

chattymin · 2025-01-15T17:47:40Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfo.kt

 import com.spoony.spoony.core.designsystem.theme.SpoonyAndroidTheme
+import kotlinx.collections.immutable.immutableListOf

 @Composable
 fun StoreInfo(


p1) 너무 외부까지 slot으로 뚫은 것 같아요.
내부에서 일부 선언해서 컴포넌트로 사용할 수 있게 해주세요

현재 Preview를 보시면 컴포넌트를 사용하는데 최소 120줄을 사용하고 있어요.
이것은 컴포넌트라고 볼 수 없을 것 같아요

넵 수정했습니다!

chattymin · 2025-01-15T17:48:39Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/component/StoreInfoItem.kt

-    subTitle: String? = null,
-    items: List<String>,
-    isMenu: Boolean = true,
+    content: @Composable () -> Unit,


p2) content는 맨 뒤에 넣어서 lambda로 쓸 수 있게 해주세요

수정했습니다!

Hyobeen-Park

진짜 찐찐막 코리

Hyobeen-Park · 2025-01-15T18:05:54Z

app/src/main/res/values/strings.xml

+    <string name="PLACE_DETAIL_STORE_INFO_MENU_TITLE">Menu</string>
+    <string name="PLACE_DETAIL_STORE_INFO_LOCATION_TITLE">Location</string>


P1: string은 snake case로 네이밍합니다!! 이름만 수정해주세요~~!!

수정했습니다!

chattymin

고생하셨습니다 🚀

Hyobeen-Park

수고하셨습니다~~🚀

Roel4990 added 2 commits January 15, 2025 22:21

[Feat/#34] StoreInfoItem UI 구현

ff9fbf1

[Feat/#34] StoreInfo UI 구현

b08299f

Roel4990 added 🧡세홍🧡 🧡세홍🧡 FEAT✨ 새로운 기능 구현 labels Jan 15, 2025

Roel4990 requested review from chattymin, Hyobeen-Park and angryPodo January 15, 2025 13:24

Roel4990 self-assigned this Jan 15, 2025

Roel4990 requested a review from a team as a code owner January 15, 2025 13:24

Roel4990 linked an issue Jan 15, 2025 that may be closed by this pull request

[FEAT] 장소 상세 페이지 StoreInfo, StoreInfoItem 구현 #34

Open

1 task

Hyobeen-Park requested changes Jan 15, 2025

View reviewed changes

chattymin requested changes Jan 15, 2025

View reviewed changes

Roel4990 added 6 commits January 16, 2025 00:58

Merge branch 'develop' into feat/#34-explore-card

9dd330b

[Feat/#34] StoreInfo 및 StoreInfoItem 전체적으로 구조 수정

9e4bb4a

[Feat/#34] StoreInfo 구조 변경 및 StoreInfoItem title strings 추출

6d6b8b6

[Feat/#34] Hairline 사용

f187f66

[Feat/#34] padding 값 수정

f8945cf

[Feat/#34] isBlurred 디폴트값 제거

193dd10

Roel4990 requested review from chattymin and Hyobeen-Park January 15, 2025 17:08

[Feat/#34] ktLintFormat 처리

a3317e5

chattymin requested changes Jan 15, 2025

View reviewed changes

Roel4990 added 2 commits January 16, 2025 03:00

[Feat/#34] StoreInfo 컴포넌트 수정

1376aff

[Feat/#34] StoreInfoItem content 위치 수정

608683a

Roel4990 requested a review from chattymin January 15, 2025 18:03

Hyobeen-Park requested changes Jan 15, 2025

View reviewed changes

chattymin approved these changes Jan 15, 2025

View reviewed changes

[Feat/#34] string snake case 으로 수정

869c28b

Roel4990 requested a review from Hyobeen-Park January 15, 2025 19:37

Hyobeen-Park approved these changes Jan 15, 2025

View reviewed changes

		<string name="PLACE_DETAIL_STORE_INFO_MENU_TITLE">Menu</string>
		<string name="PLACE_DETAIL_STORE_INFO_LOCATION_TITLE">Location</string>

[Feat/#34] 장소 상세 페이지 StoreInfo, StoreInfoItem 구현 #35

Are you sure you want to change the base?

[Feat/#34] 장소 상세 페이지 StoreInfo, StoreInfoItem 구현 #35

Conversation

Roel4990 commented Jan 15, 2025

Related issue 🛠

Work Description ✏️

Screenshot 📸

To Reviewers 📢

Hyobeen-Park 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

Choose a reason for hiding this comment

chattymin 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

chattymin 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

Hyobeen-Park left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chattymin left a comment

Choose a reason for hiding this comment

Hyobeen-Park left a comment

Choose a reason for hiding this comment