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

[Feature] 버스 리뉴얼 - 셔틀버스 시간표 개요 #459

Merged
merged 13 commits into from
Nov 6, 2024

Conversation

ThirFir
Copy link
Contributor

@ThirFir ThirFir commented Nov 5, 2024

개요

  • 버스 리뉴얼 진행
  • 셔틀 버스 시간표 목록 스크린
  • 읽기 전용(선택 불가) 칩 컴포넌트
  • 흰색 배경 Surface 컴포넌트
  • Kotlin Serialization Navigation

Screenshot 📸

image

@ThirFir ThirFir self-assigned this Nov 5, 2024
@ThirFir ThirFir requested a review from a team as a code owner November 5, 2024 14:43
Copy link
Collaborator

@Jokwanhee Jokwanhee left a comment

Choose a reason for hiding this comment

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

컴포즈 코드 재밌네요 ☺️
컴포즈 관련해서 궁금한게 많아서.. 질문좀 넣어놨습니다. !!
어프로브는 미리해놓을게요~ 템플릿 코드들 같아서~


implementation(platform(libs.compose.bom))
implementation(libs.bundles.compose.m3)

debugImplementation(libs.bundles.compose.debug.test)
androidTestImplementation(libs.compose.ui.test.manifest)

implementation("androidx.navigation:navigation-compose:2.8.3")
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 이렇게 쓴 이유가 없으면, toml 파일 사용해주시면 감사하겠습니다 🙇🏻

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

지금 toml에서 2.7.7 쓰고 있는데 navigation 2.8.0 부터 Kotlin Serializable을 사용한 방식이 도입됐습니다 !
근데 막 올리면 2.7.7 사용하던 비즈니스 모듈에 무슨 문제가 생길지 예상이 안돼서 분리했습니다 ㅠㅠ


BusNavigation(
modifier = Modifier.fillMaxSize(),
navController = rememberNavController(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

BusNavigation 함수에서도 default value로 rememberNavController 넣어줬던데, default value 설정하고 밖에서 넣어주면 무슨 이점이 있나요?

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

default로 두면 Preview할 때 navController를 주지 않아도 되는 점이 장점인 거 같아용
굳이 Activity에서 만들어서 넘길 필요는 없는 것 같긴해요 (아직까지는 그렇게만 느꼈어요)

@@ -16,7 +21,10 @@ class Bus2Activity : AppCompatActivity() {
setContentView(R.layout.activity_bus2)
findViewById<ComposeView>(R.id.compose_view_bus).setContent {
MaterialTheme {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Theme 설정을 KoinTheme 로 하지 않은 이유가 있나요? 👀

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

아직 뷰 만드는 작업만 해서 그렇습니다 !!
나중에 바꿀게욥

style = KoinTheme.typography.bold20
)
Spacer(modifier = Modifier.height(8.dp))
LeadingIconText(
Copy link
Collaborator

Choose a reason for hiding this comment

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

LeadingIconText 컴포저블 내부 확인해보니 아이콘 리소스를 파라미터로 기본형 타입 넘겨주더라도 리컴포지션이 발생하더라구요! 그래서 저도 StableIcon라고 만들어놓으면서 @stable 을 명시해줬어요 🥲
아니면 Icon 말고 Image 를 사용해야 하더라구요...

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

오잉 상위 리컴포지션 발생 시 LeadingIconText도 같이 리컴포지션 되나요 ?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

어머 죄송합니다. 이게 제 StableIcon에는 modifier 을 넣어주어서 리컴포지션이 발생했었던 이슈였네요..
저 LeadingIconText 는 상관 없을 것 같습니다 😅

viewModel: BusTimetableViewModel = hiltViewModel()
) {

var selectedTimetableTypeTabIndex by rememberSaveable { mutableIntStateOf(0) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

까먹고 있었던 코드인데, 다시 상기해주셔서 감사합니다 👀 리팩토링할 때 넣어봐야겠어요


stickyHeader {
KoinTabRow(
titles = BusType.entries.map { stringResource(it.titleRes) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

(단순 질문 🙋) 컴포넌트로 만들때, 컬렉션을 넘겨줄 경우 따로 Immutable 사용은 안하나요?

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

오.. 이거는 신경못썼던 부분이네요 👍
divider랑 indicator가 람다라서 @stable을 붙이기에는 애매한 거 같고... List를 ImmutableList로 설정하는 게 맞는 거 같은데... 근데 그러자니 모든 모듈에 ImmutableList dependency를 추가해야하고... 어렵다...
좀 더 생각해볼게여... 🥺

) {

var selectedRouteTypeIndex by rememberSaveable { mutableIntStateOf(ShuttleBusRouteType.ALL.ordinal) }
val context = LocalContext.current
Copy link
Collaborator

Choose a reason for hiding this comment

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

context 사용할 때, 함수 파라미터로 옮기면 더 확장성이 있을 것 같은데... 혹시 이렇게 사용하는 이유가 따로 있는걸까요? (맨 첫 화면이라서 그런가요? 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context를 외부에서 주입하자는 말씀이신가요??
context를 사실 컴포저블들에서 별로 사용할 일이 없어서 전 이렇게 하는데, 혹시 어떤 점에서 확장성이 좋을 것 같다는 말씀이신가용??

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Composable
fun Screen(
  context: Context= LocalContext.current
) { ... }

저는 위처럼 많이 사용하긴 하는데, 파라미터로 호출자가 직접 context를 전달할 수 있어서 유연하지 않을까? 생각해봤는데, 컴포즈에서는 주로 어떤 방식을 사용하는 지 궁금해서 물어봤었어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

주로 어떤 방식을 쓰는진 잘 모르겠는데, 제가 봐온 코드에서는 거의 Composable안에서 선언해줬던 것 같습니답

import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.material.icons.Icons
Copy link
Collaborator

Choose a reason for hiding this comment

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

material2를 사용하면 Icon 가지고올 수 있어서 개꿀인것같아요. 이번에 쓰면서 m3 에 없길래 당황했었는데 🫠

Copy link
Contributor Author

@ThirFir ThirFir Nov 5, 2024

Choose a reason for hiding this comment

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

이거 m2에여??
저 bus에 m2 추가안했는데 ....? 🧐

Copy link
Collaborator

Choose a reason for hiding this comment

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

오잉 저 라이브러리 material2 아닌가? 뭐지 ㅋㅋ 근데 m2 추가는 안되어있는데 m3 추가하면 저것도 생기는건가.. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

https://mvnrepository.com/artifact/androidx.compose.material3/material3/1.3.1

한번 찾아봤는데 `androidx.compose.material:material3' 요 패키지에 아이콘이랑 ripple 같이 Material2, 3에서 공통적으로 쓰이는 요소들은 그대로 쓰이나봐요 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

대신 찾아드립니다 ㄷㄷ

@ThirFir ThirFir force-pushed the feature/bus-renewal branch 2 times, most recently from 8dce681 to ffe957d Compare November 5, 2024 19:44
Copy link
Contributor

@nodobi nodobi 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 +20 to +29
/**
* 선택 불가한 텍스트 칩
* @param title 텍스트
* @param modifier Modifier
* @param containerColor 칩 배경색
* @param textStyle 텍스트 스타일
* @param shape 칩 모양
* @param contentPadding 칩 내부 padding
*/
@Composable
Copy link
Contributor

Choose a reason for hiding this comment

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

컴포넌트 추가할 때 마다 주석 달아주시는거 너무 좋아요!!
매번 감사하게 가져다가 쓰고 있습니다 🙇‍♂️

Comment on lines +6 to +12
internal enum class BusType(
@StringRes val titleRes: Int
) {
SHUTTLE(R.string.tab_shuttle),
EXPRESS(R.string.tab_express),
CITY(R.string.tab_city),
}
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 Author

@ThirFir ThirFir Nov 6, 2024

Choose a reason for hiding this comment

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

레전드 enum Lover에요

import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.material.icons.Icons
Copy link
Contributor

Choose a reason for hiding this comment

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

https://mvnrepository.com/artifact/androidx.compose.material3/material3/1.3.1

한번 찾아봤는데 `androidx.compose.material:material3' 요 패키지에 아이콘이랑 ripple 같이 Material2, 3에서 공통적으로 쓰이는 요소들은 그대로 쓰이나봐요 🙄

@ThirFir ThirFir merged commit cff1b23 into develop Nov 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants