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

feat: 웹소설 여러 플랫폼 대응 #531

Merged
merged 21 commits into from
Jan 15, 2025
Merged

feat: 웹소설 여러 플랫폼 대응 #531

merged 21 commits into from
Jan 15, 2025

Conversation

junseo511
Copy link
Member

@junseo511 junseo511 commented Jan 9, 2025

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

  • 컴포즈 관련 의존성 추가
  • 컴포즈가 가진 기본 클릭 효과를 없애는 확장 컴포넌트 추가
  • 기존 플랫폼 로딩 로직 개선
  • 컴포즈 컴포넌트 추가

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

CPU 점유율 상으로 39% -> 33% (약 18%차이)의 성능 개선이 있었습니다

Screen_recording_20250110_030851.mp4

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

컴포즈 가보자구요~

@junseo511 junseo511 added 🍯 [FEAT] 새로운 기능을 개발합니다. 🔮 법사 준서 아카데미의 천재 마법사 labels Jan 9, 2025
@junseo511 junseo511 added this to the 1차 스프린트 milestone Jan 9, 2025
@junseo511 junseo511 requested a review from m6z1 January 9, 2025 18:23
@junseo511 junseo511 self-assigned this Jan 9, 2025
@junseo511 junseo511 requested review from librarywon and m6z1 and removed request for m6z1 and librarywon January 9, 2025 18:26
Copy link
Member

@m6z1 m6z1 left a comment

Choose a reason for hiding this comment

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

고생하셔써요 컴포즈 신기하구만요
세팅도 감사합니다 👍

import com.into.websoso.common.util.getS3ImageUrl
import com.into.websoso.ui.novelInfo.model.PlatformModel

@OptIn(ExperimentalLayoutApi::class)
Copy link
Member

Choose a reason for hiding this comment

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

a: 옹 이건 무슨 어노테이션인가여 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Jetpack Compose에서 실험적으로 운용하는 컴포넌트를 말합니다!
저희가 사용하는 컴포즈 버전이 변동된다면 수정사항이 생길 수 있습니다.
Flow는 다른 Grid 컴포넌트와 달리 높이가 가변적이어서 Chip 등을 표현할 때 유리합니다 :)

Comment on lines +28 to +31
shape = RoundedCornerShape(12.dp),
width = 1.dp,
color = Color.Transparent,
)
Copy link
Member

Choose a reason for hiding this comment

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

a: 오 컴포즈 대박이다

<LinearLayout
android:id="@+id/ll_novel_info_platforms"
android:layout_width="wrap_content"
<androidx.compose.ui.platform.ComposeView
Copy link
Member

Choose a reason for hiding this comment

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

a: 짱 신기

@junseo511 junseo511 requested a review from librarywon January 14, 2025 05:46
platformImage: String,
onClick: () -> Unit,
) {
AsyncImage(
Copy link
Member

@s9hn s9hn Jan 14, 2025

Choose a reason for hiding this comment

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

�r: AsyncImage는 따로 컴포넌트로 분리하면 좋을 것 같아요!
common - ui 쪽에 component 하나 뚫어서 공용 컴포넌트로 만들어주시죠!

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
Member Author

Choose a reason for hiding this comment

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

아 좋은거같습니다 제가 한 번 맛깔나게 말아보죠 👍

Copy link
Contributor

@librarywon librarywon left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다!

@@ -68,6 +72,7 @@ dependencies {
implementation("androidx.appcompat:appcompat:1.6.1")
implementation("com.google.android.material:material:1.11.0")
implementation("androidx.constraintlayout:constraintlayout:2.1.4")
implementation("androidx.compose.ui:ui-android:1.7.6")
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 아래 컴포즈를 모아두신 것 같은데 이 친구만 여기 있네요?

Copy link
Member Author

Choose a reason for hiding this comment

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

헐 그러게요 버전 카탈로그 풀땡기고 옮겨야겠어요

import androidx.compose.ui.Modifier

@Composable
fun Modifier.clickableWithoutRipple(
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 이건 모디파이어의 확장함수인데 파일 이름이 Modifier 익스텐션이 좋지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 것들도 일종의 컴포넌트라고 생각해서 Modifier 확장함수만 따로 묶는건 안하려고 했는데 어떻게 생각하시는지...?!

Copy link
Contributor

@librarywon librarywon Jan 14, 2025

Choose a reason for hiding this comment

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

Modifier 함수가 컴포넌트라고 말씀하시는건 동의하기가 어렵네요

Copy link
Member Author

Choose a reason for hiding this comment

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

'UI를 구성하거나 특정 기능을 수행하는 독립적인 요소'라는 컴포넌트의 정의를 생각해 본다면 Modifier의 확장함수 역시 컴포넌트라고 정의할 수 있다고 생각합니다! 그리고 만약 특정 기능을 수행하는 확장 컴포넌트를 사용하고 싶은 경우에, Modifier의 확장함수임을 인지하지 못한 상태로 해당 기능을 찾으려고 한다면 오히려 가독성 측면에서 떨어진다고 생각했습니다 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

제가 Modifier.clickableWithoutRipple가 독립적으로 존재 가능한가요? 컴포넌트의 속성으로 상당히 의존적인 친구 같지 않나요?

Modifier.clickableWithoutRipple 기능을 포함한 새로운 이미지 컴포넌트를 만든다면 컴포넌트라고 정의 가능해보입니다

Copy link
Member Author

Choose a reason for hiding this comment

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

음...! 의존적이라는 측면에서는 수긍하고 파일을 분리하겠습니다!! 하지만 Modifier의 확장함수임에도 독자적인 기능이 있는 경우에도 고려해보면 좋을 것 같아요. 확장 컴포넌트를 사용하는 사람들이 Modifier의 확장함수인지 인지하지 못하는 경우도 고려해봐야 할 것 같구요!

platformImage: String,
onClick: () -> Unit,
) {
AsyncImage(
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 10 to 12
enabled: Boolean = true,
interactionSource: MutableInteractionSource = MutableInteractionSource(),
onClick: () -> Unit,
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 그리고 이 파라미터들은 꼭 필요한건가요? enable 속성을 false로 하는 경우가 있는지 interactionSource를 새로 받거나

Copy link
Member Author

Choose a reason for hiding this comment

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

interactionSource는 빼도 좋을 것 같네요! 사실 사용할 일이 잘 없는 친구라서요! enabled 같은 경우에는 종종 버튼을 비활성화한다거나 하는 기능 등에서 필요 할 것 같아요!! 일부 조건에서 버튼 회색 + 클릭 비활성화 등이 필요한 경우에요!

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
Member Author

@junseo511 junseo511 Jan 14, 2025

Choose a reason for hiding this comment

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

아예 빼두지 않는 편이 나은 것 같다는 뜻일까요...?! 컴포즈의 기본 클릭 이펙트를 없앨 indication = null는 각각의 clickable에서 사용할 때마다 직접 붙이는 것이 나을까요??? 이건 저스트 질문!!

Copy link
Member

Choose a reason for hiding this comment

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

근데 우리 모든 상호작용 UX는 호버없이 구현하기로 픽스됐었나요?

Copy link
Member Author

@junseo511 junseo511 Jan 14, 2025

Choose a reason for hiding this comment

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

현재 앱 내의 거의 모든 상호작용이 리플효과 없이 이루어지고 있지 않나요?

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
Member

Choose a reason for hiding this comment

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

그럼 전 빼도 좋을듯!

width = 1.dp,
color = Color.Transparent,
)
.clickableWithoutRipple { onClick() },

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:chain-method-continuation reported by reviewdog 🐶
Unexpected newline before '.'

Copy link
Contributor

@librarywon librarywon left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다!

@@ -216,6 +190,7 @@
android:background="@color/gray_50_F4F5F8"
app:isVisible="@{novelInfoViewModel.uiState.novelInfoModel.isAttractivePointsExist}"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintHorizontal_bias="0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 오 이거 필요한가용?

@junseo511 junseo511 requested a review from s9hn January 15, 2025 03:10
feat: 웹소소 컴포즈 대응 디자인 시스템 추가
@junseo511 junseo511 merged commit c3d762c into develop Jan 15, 2025
1 check failed
}

@Composable
fun ProvideWebsosoTypography(typography: WebsosoTypography, content: @Composable () -> Unit) {

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:function-signature reported by reviewdog 🐶
Newline expected after opening parenthesis

}

@Composable
fun ProvideWebsosoTypography(typography: WebsosoTypography, content: @Composable () -> Unit) {

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:function-signature reported by reviewdog 🐶
Parameter should start on a newline

}

@Composable
fun ProvideWebsosoTypography(typography: WebsosoTypography, content: @Composable () -> Unit) {

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:function-signature reported by reviewdog 🐶
Newline expected before closing parenthesis

caption: TextStyle = this.caption,
button: TextStyle = this.button,
label: TextStyle = this.label,
): WebsosoTypography = WebsosoTypography(

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:function-signature reported by reviewdog 🐶
Newline expected before expression body

fontWeight: FontWeight,
fontSizeDp: Dp,
lineHeightDp: Dp,
): TextStyle {

Choose a reason for hiding this comment

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

🚫 [ktlint] standard:function-expression-body reported by reviewdog 🐶
Function body should be replaced with body expression

@m6z1 m6z1 deleted the feat/518 branch January 23, 2025 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍯 [FEAT] 새로운 기능을 개발합니다. 🔮 법사 준서 아카데미의 천재 마법사
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 웹소설 여러 플랫폼 대응
4 participants