[Feature/#866] 오늘의 솝마디 신규 Feature 구현 #867
Conversation
|
feature/fortune/src/main/java/org/sopt/official/feature/fortune/MainScreen.kt
Outdated
Show resolved
Hide resolved
| <activity | ||
| android:name=".feature.fortune.FortuneActivity" | ||
| android:exported="true" | ||
| android:launchMode="singleTop" /> |
There was a problem hiding this comment.
exported="true" 로 해둔 이유는 추후 앱 알림을 클릭시 해당 화면으로 이동시킬 예정이어서 그렇습니다 :)
| @Composable | ||
| internal fun HomeRoute( | ||
| paddingValue: PaddingValues, | ||
| navigateToFortuneDetail: (String) -> Unit, | ||
| ) { | ||
| val date = remember { getTodayInfo() } | ||
|
|
||
| HomeScreen( | ||
| paddingValue = paddingValue, | ||
| date = date, | ||
| navigateToFortuneDetail = { | ||
| navigateToFortuneDetail(date) | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| @Composable | ||
| private fun HomeScreen( | ||
| paddingValue: PaddingValues, | ||
| date: String, | ||
| navigateToFortuneDetail: () -> Unit = {}, | ||
| ) { |
| fun getTodayInfo(): String { | ||
| val today = LocalDate.now() | ||
|
|
||
| val monthDay = today.format(DateTimeFormatter.ofPattern("M월 d일")) | ||
| val dayOfWeek = today.dayOfWeek.getDisplayName( | ||
| TextStyle.FULL, | ||
| Locale.KOREAN | ||
| ) | ||
|
|
||
| return "$monthDay $dayOfWeek" | ||
| } |
There was a problem hiding this comment.
이 부분 말씀하시는 거지요?! 지금은 이렇게 두고 나중에 로직 복잡해지면 뷰모델로 빼도 괜찮을 것 같아요!
| Icon( | ||
| imageVector = Icons.Filled.Close, | ||
| contentDescription = null, | ||
| modifier = Modifier | ||
| .padding(start = 8.dp, top = 2.dp, bottom = 2.dp) | ||
| .padding(8.dp) | ||
| .clickable { | ||
| // TODO: Navigate to NotificationActivity | ||
| }, |
There was a problem hiding this comment.
Icon에 clickable을 두어도 버튼처럼 활용할 수 있는데 IconButton 컴포저블을 활용하면 좋은 점이 어떤 것들이 있을까요?
There was a problem hiding this comment.
IconButton을 사용한다면 장점은 아래와 같다고 생각합니다.
- Button임을 명시적으로 표현할 수 있다. -> 개발자가 한눈에 보기 좋음
- 기본 패딩값이 이미 지정되어있다.
하지만, 저는 2번째 장점이 이 구현에서 잘못되었다고 생각해서 Icon을 사용했습니다.
| Icon | IconButton |
|---|---|
 |
 |
두 사진을 비교해본다면 IconButton이 topBar 가 더 두꺼운 것을 볼 수 있습니다.
그 이유는 IconButton은 기본적으로 48x48dp으로 설정되어있습니다.
하지만, 피그마상으로 아이콘은 24의 기본 크기에 주변에 8dp만큼의 패딩을 가지고있는 즉, 32의 크기입니다.
그렇기 때문에 디자이너의 요구사항을 맞추고자 IconButton이 아닌 커스텀이 가능한 Icon을 사용했습니다
feature/fortune/src/main/java/org/sopt/official/feature/fortune/MainScreen.kt
Outdated
Show resolved
Hide resolved
| composable<FortuneDetail> { backStackEntry -> | ||
| val items = backStackEntry.toRoute<FortuneDetail>() | ||
| FortuneDetailRoute( | ||
| date = items.date | ||
| paddingValue = paddingValue, | ||
| date = items.date, | ||
| navigateToFortuneAmulet = { | ||
| navController.navigate(FortuneAmulet) | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| composable<FortuneAmulet> { | ||
| FortuneAmuletRoute( | ||
| paddingValue = paddingValue, | ||
| navigateToHome = { | ||
| // TODO: Navigate to Home | ||
| } |
There was a problem hiding this comment.
navHost에겐 네비게이션 그래프와 이동할 수 있는 목적지만 알려줘도 좋을 것 같아요
| AsyncImage( | ||
| model = "https://어쩌구저쩌구/test_fortune_card.png", // 서버에서 받아온 이미지 | ||
| contentDescription = null, | ||
| modifier = Modifier | ||
| .padding(horizontal = 33.dp) | ||
| .fillMaxHeight(0.55f) | ||
| ) |
There was a problem hiding this comment.
추후 다른 라이브러리를 사용하게 된다면..? 해당 컴포넌트를 사용하는 모든 곳이 수정되어야합니다!
따라서 저는 AsyncImage 같은것은 한번 더 Wrapping한 컴포넌트를 공용으로 사용합니다.
There was a problem hiding this comment.
이 부분은 생각 못해봤었는데 덕분에 생각해보게 됐네요!!
감사합니다 :)
There was a problem hiding this comment.
그런데 AsyncImage 함수 네이밍은 좋은 것 같아서
@Composable
fun AsyncImage(
model: Any?,
contentDescription: String?,
modifier: Modifier = Modifier
) {
io.coil.kt.AsyncImage(
model = model,
contentDescription = contentDescription,
modifier = modifier
)
}정도로 적어도 좋을 것 같아요 👍🏻
There was a problem hiding this comment.
하나 우려되는 것은 AsyncImage라는 이름이 너무 유명해져있는 상태라 생각됩니다. 추후 코드를 보는 사람이 coil 라이브러리에 해당하는 컴포넌트라고 헷갈릴 수 있을 것 같아요. AsyncImageLoader는 어떨까요??
@l2hyunwoo
There was a problem hiding this comment.
UI가 async 와 같은 구현방식까진 알 필요 없다고 생각합니다.
그냥 Image와 차별을 두고 싶으면 파라미터로 구분을 주던가 추상화된 네이밍으로 덮어도 좋을 것 같아요
There was a problem hiding this comment.
- AsyncImage라는 함수 네이밍 그 자체로 보면 네트워크나 로컬 소스(캐시 등등)에서 이미지를 비동기적으로 가져오는 것을 이보다 더 잘 표현할 수 없다고 생각해요 (NetworkImage는 솔직히 좀 짜칩니다 개인적으로는)
- async와 같은 구형방식까진 알 필요는 없어도 material3에서 제공하는 컴포넌트와는 확연히 구분해주고 싶다는 생각은 있어요. 이것보다 더 좋은 네이밍 있으면 추천해주시면 좋을 것 같습니다.
There was a problem hiding this comment.
비동기이미지(
model = model,
contentDescription = contentDescription,
modifier = modifier
)
There was a problem hiding this comment.
한글날이 2주남았는데 머종세왕님 little 서운하실듯
There was a problem hiding this comment.
우선 머지해야 세훈님이 다음 작업 하시기 수월해서 머지하겠습니다!
이 부분은 계속 고민해보고 다음번에반영할게요
| fun MainScreen( | ||
| navController: NavHostController = rememberNavController(), | ||
| ) { |
There was a problem hiding this comment.
screen보단 기초, 뼈대, 제일 베이스의 의미가 내포된 네이밍이면 좀 더 역할에 어울릴 것 같아요
There was a problem hiding this comment.
Screen이라는 단어를 빼기에는 TopBar, SnackBar, BottomBar등 이런 부분은 화면을 나타내는 요소라고 생각합니다.
그래서 Main이라는 단어를 Foundation으로 변경하여 모든 화면의 근간이 된다 라는 의미로 네이밍을 변경하였습니다.
There was a problem hiding this comment.
그렇다면 본 Screen은 screen으로써 어떤 시나리오를 기대할 수 있나요?
There was a problem hiding this comment.
topBar, bottomBar등의 UI를 나타낼 수 있고, bottomSheet, snackBar 등 화면에 나타나는 구성요소들을 띄우는 역할을 한다고 생각합니다.
There was a problem hiding this comment.
FoundationScreen의 Preview 및 UI Test로 각각의 컴포넌트가 등장함을 확인할 수 있나요?
There was a problem hiding this comment.
어.. 제가 테스트는 안해봐서 모르겠고, Preview의 경우에는 topBar, bottomBar 처럼 화면에 나타나는 것은 보입니다.
다만 스낵바와 같이 호출되어야만 나타나는 것의 경우에는 안보여요
There was a problem hiding this comment.
우선 머지해야 세훈님이 다음 작업 하시기 수월해서 머지하겠습니다!
이 부분은 계속 고민해보고 다음번에반영할게요
l2hyunwoo
force-pushed
the
feature/#866-today-say-ui
branch
from
6ad63de
to
6b19a2f
Compare
What is this issue?
Reference
end.mp4
To Reviewer