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/#313] 직무 필터링 추가 #318

Merged
merged 31 commits into from
Jan 4, 2025

Conversation

Hyobeen-Park
Copy link
Member

@Hyobeen-Park Hyobeen-Park commented Dec 30, 2024

⛳️ Work Description

  • 직무 필터링 추가
  • 계획 필터링 UI 수정

📸 Screenshot

Screen_Recording_20241231_041604_terning.mp4

📢 To Reviewers

  • 개큰지각 죄송합니다...😭😭😭😭
  • 바텀시트 height를 계획 필터링을 기준으로 고정하고 싶었는데 그게 잘 안되더라구요.. 그래서 일단 3/4f로 지정해두었습니다!!

@Hyobeen-Park Hyobeen-Park added the FEAT ✨ 새로운 기능 구현 label Dec 30, 2024
@Hyobeen-Park Hyobeen-Park added this to the 3차 스프린트 작업 milestone Dec 30, 2024
@Hyobeen-Park Hyobeen-Park self-assigned this Dec 30, 2024
Copy link
Member

@boiledEgg-s boiledEgg-s left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다!! 얼른 머지해서 사용해보고 싶네요ㅎㅋㅎㅋㅎ


@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun HomeFilteringBottomSheet(
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

Choose a reason for hiding this comment

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

오 진짜 너무 좋은 생각이에요!! 바아로 적용해보겠습니다

Comment on lines 204 to 206
if (isCheckBoxChecked) {
selectedIndex = -1
}
Copy link
Member

Choose a reason for hiding this comment

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

isCheckBoxChecked의 변화가 감지된 경우에만 selectedIndex의 값이 바뀌도록 설정해주면 불필요하게 변수가 초기화되는 것을 방지할 수 있을 것 같네요!

var currentFilteringInfo by remember {
mutableStateOf(defaultFilteringInfo)
}
val pagerState = rememberPagerState { 2 }
Copy link
Member

Choose a reason for hiding this comment

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

하드코딩보단 2라는 값이 발생한 이유? 그 정보를 여기에 담아주면 코드를 이해하기 편하고 나중에 수정하기도 편해질 것 같네요!
제가 이해한 바론 2는 filterType의 크기 같은데 크기 정보로 대체하면 좋을 것 같습니당~

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 151 to 152
else -> HomeFilteringInfo(null, null, null, null, JobType.TOTAL.stringValue)
}
Copy link
Member

Choose a reason for hiding this comment

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

아무래도 파라미터가 많아져서 named arguments를 사용해서 명시해주는 게 좋을 것 같아요..!

Comment on lines 53 to 58
fun HomeFilteringBottomSheet(
modifier: Modifier = Modifier,
defaultFilteringInfo: HomeFilteringInfo,
onDismiss: () -> Unit,
onChangeButtonClick: (String?, String?, Int?, Int?, String) -> Unit,
) {
Copy link
Member

Choose a reason for hiding this comment

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

modifier는 기본값이 있으므로 가장 아래로 내려주세요!

Comment on lines 100 to 102
}
})
}
Copy link
Member

Choose a reason for hiding this comment

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

진짜 사소한 거긴하지만... 이렇게 해주면 더 코드가 예뻐질 것 같아요...!

Suggested change
}
})
}
}
}
)
}

Comment on lines 236 to 240
private fun checkButtonEnable(currentFilteringInfo: HomeFilteringInfo): Boolean =
with(currentFilteringInfo) {
(grade == null && workingPeriod == null && startYear == null && startMonth == null)
|| (grade != null && workingPeriod != null && startYear != null && startMonth != null)
}
Copy link
Member

Choose a reason for hiding this comment

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

아래와 같은 방식으로도 바꿀 수 있을 것 같아요..! 이건 그냥 참고만 해주세요!

Suggested change
private fun checkButtonEnable(currentFilteringInfo: HomeFilteringInfo): Boolean =
with(currentFilteringInfo) {
(grade == null && workingPeriod == null && startYear == null && startMonth == null)
|| (grade != null && workingPeriod != null && startYear != null && startMonth != null)
}
private fun checkButtonEnable(currentFilteringInfo: HomeFilteringInfo): Boolean =
with(currentFilteringInfo) {
listOf(grade, workingPeriod, startYear, startMonth).all { it == null } ||
listOf(grade, workingPeriod, startYear, startMonth).none { it == null }
}

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 +46 to +51
modifier = modifier
.fillMaxWidth()
.wrapContentHeight()
.padding(bottom = 60.dp, start = 23.dp, end = 23.dp),

) {
Copy link
Member

Choose a reason for hiding this comment

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

파라미터가 3개 이상 넘어가면 줄바꿈을 해주는 게 가독성에 좋을 것 같아요....!!

Suggested change
modifier = modifier
.fillMaxWidth()
.wrapContentHeight()
.padding(bottom = 60.dp, start = 23.dp, end = 23.dp),
) {
modifier = modifier
.fillMaxWidth()
.wrapContentHeight()
.padding(
bottom = 60.dp,
start = 23.dp,
end = 23.dp),
) {

Comment on lines 194 to 202
@Composable
fun ChangePlanFilteringRadioGroup(
optionList: List<Int>,
initOption: Int,
isCheckBoxChecked: Boolean,
onButtonClick: (Int) -> Unit,
columns: Int = 3,
modifier: Modifier = Modifier,
) {
Copy link
Member

Choose a reason for hiding this comment

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

순서랑 List 바꿔주시면 감사하겠습니당...!!

Suggested change
@Composable
fun ChangePlanFilteringRadioGroup(
optionList: List<Int>,
initOption: Int,
isCheckBoxChecked: Boolean,
onButtonClick: (Int) -> Unit,
columns: Int = 3,
modifier: Modifier = Modifier,
) {
@Composable
fun ChangePlanFilteringRadioGroup(
optionList: ImmutableList<Int>,
initOption: Int,
isCheckBoxChecked: Boolean,
onButtonClick: (Int) -> Unit,
modifier: Modifier = Modifier,
columns: Int = 3,
) {

Comment on lines 74 to 76
modifier = modifier
.fillMaxWidth()
.fillMaxHeight(3 / 4f),
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

@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 +127 to +129
.padding(top = 16.dp),
) { currentPage ->
when (currentPage) {
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

Choose a reason for hiding this comment

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

뭐지뭐지 했는데 이게 범인이었던..ㅎㅎ

Comment on lines +252 to +263
@Composable
private fun GetPagerHeight(
onHeightMeasured: (Int) -> Unit,
) {
PlanFilteringScreen(
currentFilteringInfo = HomeFilteringInfo(null, null, null, null, "total"),
modifier = Modifier
.onGloballyPositioned {
onHeightMeasured(it.size.height)
}
)
}
Copy link
Member

Choose a reason for hiding this comment

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

onGloballyPositioned가 크기를 측정하는 친구인가요?
정말... 대단하네요....ㄷㄷㄷ

Copy link
Member Author

Choose a reason for hiding this comment

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

네네!! 위치랑 크기 정보를 알 수 있다고 하더라구요..!! 사실 이게 PlanFilteringScreen을 한번 그려보고 높이를 측정한건데 이렇게 안하고는 높이를 알아낼 방법을 찾아내지 못해서,,, 그냥 냅다 해버렸어요😂

Comment on lines +247 to +250
with(currentFilteringInfo) {
listOf(grade, workingPeriod, startYear, startMonth).all { it == null || it == 0 } ||
listOf(grade, workingPeriod, startYear, startMonth).none { it == null || it == 0 }
}
Copy link
Member

Choose a reason for hiding this comment

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

어머 이 부분은 제가 놓쳤네요 ! 😂 0일 때도 확인하는 조건 넣어주셔서 감사합니당~

@Hyobeen-Park Hyobeen-Park merged commit f93f578 into develop Jan 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FEAT ✨ 새로운 기능 구현
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEAT] 홈 필터링 재설정 뷰 직무 카테고리 구현
3 participants