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

[REFACTOR/#212] 다이얼로그 / 리팩토링 #213

Merged
merged 27 commits into from
Sep 6, 2024

Conversation

boiledEgg-s
Copy link
Member

@boiledEgg-s boiledEgg-s commented Sep 1, 2024

⛳️ Work Description

리팩토링

  • 다이얼로그 패키지화
  • 다이얼로그 내 스크랩 관리
  • 캘린더 연동

2차 스프린트

  • 컬러 라디오버튼
  • 화면별 버튼
  • 색상 수정 버튼
  • 텍스트 높이 3줄로 고정

📸 Screenshot

스크랩 수정 (꽤 많이 수정됨)

default.mp4

스크랩 취소 (마진 정도만 바뀜)

default.mp4

📢 To Reviewers

  • 따로 패키지로 빼서 로직 분리만 하려고 했는데 다이얼로그가 생각보다 많이 바뀌어서 UI를 쬐~끔 바꿨습니다. 이 이상은 따로 이슈파서 진행하도록 할게요!!
  • 이걸로 모든 화면의 다이얼로그를 대체해야 하는데, 여러분들이 구현한 코드를 일일이 보고 고치기엔 좀 무리일 것 같아요,,
    잘 살펴보고 혹시 본인이 구현한 코드에 적용이 힘들 것 같다 싶으면 코멘트에 그렇게 생각한 부분과 이유를 적어주세요!! 전부 다 반영해서 수정하도록 할게요!
  • 조금 더 자세하게 설명해놓은 노션 페이지임다! 보러가기

-------------- 9월 3일 변경사항 --------------

  • 참지 못하고 다 구현해버렸습니다!
  • 내부 로직과 UI만 손봤기 때문에 사용법은 크게 달라지지 않았을거에요!! 위에 첨부한 노션 보시면 될 것 같습니다~
  • 불필요한 리컴포지션이 좀 발생하는 것 같은데, 이유 찾으시는 분에겐... 칭찬해드릴게요
  • 이외에도 미흡한 부분 날카롭게 지적해주세요!

Copy link
Member

@leeeyubin leeeyubin 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 +101 to +104
with(viewModel){
updateScrapId(scrapId)
updateScrapCancelDialogVisibility(true)
}
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
Contributor

@arinming arinming left a comment

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.

요기 import 정리 부탁드려요!

Comment on lines +8 to +11
@Composable
fun TextStyle.getFixHeightByMaxLine(maxLine: Int): Dp = with(LocalDensity.current) {
lineHeight.toDp() * maxLine
}
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 +42 to +43
val interactionSource = remember { MutableInteractionSource() }
val isPressed by interactionSource.collectIsPressedAsState()
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.

복붙 코드입니다ㅎㅎ 샤라웃 투 유빈

Comment on lines 91 to 94
Image(
painter = painterResource(id = R.drawable.ic_color_check),
contentDescription = ""
)
Copy link
Contributor

Choose a reason for hiding this comment

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

TerningImage 사용하시는 거 어떨까요!?

TerningImage( painter = R.drawable.ic_color_check, )

Copy link
Member Author

@boiledEgg-s boiledEgg-s Sep 4, 2024

Choose a reason for hiding this comment

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

지난 회의때 contentDescription을 통일하지 말고 직접 쓰자는 의견이 나왔었어요! 하드코딩을 하더라도 채워넣기로 정했습니다!! 8월 4주차 안드회의 보러가기

이 부분은 TerningImage는 안쓰더라도 고치긴 해야겠네요!!

Copy link
Member

@leeeyubin leeeyubin 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 3 to 5
sealed class ScrapDialogSideEffect{
data class ShowToast(val message: Int): ScrapDialogSideEffect()
data object DismissDialog : ScrapDialogSideEffect()
Copy link
Member

Choose a reason for hiding this comment

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

@StringRes 어노테이션 넣어주면 더 좋을 것 같아요!

Comment on lines +40 to +43
},
verticalArrangementSpace = 6.dp,
horizontalArrangementSpace = 0.dp,
modifier = modifier.wrapContentSize()
Copy link
Member

Choose a reason for hiding this comment

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

여기 0dp가 기본 값이 아니라서 넣어준 건가용..?

Copy link
Member Author

Choose a reason for hiding this comment

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

넵! RadioButtonGroups 컴포넌트를 만들 때 기본값을 안받도록 구현해서 직접 넣어줘야됩니다!!

Copy link
Member

@Hyobeen-Park Hyobeen-Park left a comment

Choose a reason for hiding this comment

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

다이얼로그 진짜 최고다!!! 수고하셨습니다😊

}
}

val swipeModifier = Modifier.pointerInput(Unit) {
Copy link
Member

Choose a reason for hiding this comment

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

우왕 swipe 기능까지 구현하다니!! 짱이다👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

어라,, 이게 왜 여기있지?

)
val internalBoxModifier = Modifier
.padding(4.dp)
.then(
Copy link
Member

Choose a reason for hiding this comment

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

오호!! 이렇게도 구현이 가능하군요!!!

# Conflicts:
#	feature/src/main/java/com/terning/feature/intern/component/InternInfoRow.kt
@boiledEgg-s boiledEgg-s merged commit 82d547f into develop Sep 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REFACTOR ♻️ 전면 수정 석준💜 석준
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] 다이얼로그 / 리팩토링
4 participants