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

Surface 구현 #6

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Surface 구현 #6

merged 5 commits into from
Jul 30, 2024

Conversation

kangyuri1114
Copy link
Member

image

Surface를 구현했습니다.

box, button, radio button, check box, toggle 등에서 모두 필요할 것 같아 surface 먼저 구현했습니다.
material의 surface랑 YDS를 많이 참고했는데요

기존 YDS랑 달라진 점은

  • semantics 확장함수의 isContainer 가 deprecated 되었다고 뜨더라고요. material에서는 다음과 같이 사용 중이길래 저도 그렇게 수정했습니다.
    .semantics(mergeDescendants = false) { @Suppress("DEPRECATION") isContainer = true }

  • 기존 YDS에는 기본 surface, clickable surface 두 종류 뿐이고, checkbox나 toggle같은 곳에서는 직접 구현 중이었는데, surface 만드는 김에 material 처럼 surface, clickable surface, seleted surface, toggleable surface 4종류 구현해두었습니다. 추후 구현시에 surface를 활용하면 될 것 같아요

  • 주석을 추가했습니다. 이런 식으로 쓰면 될지..고민이네요... 모든 params를 다 적어야 하는 게 좋을까요??

  • surface의 preview는 굳이.. 있을 필요가 있나 싶어 필요가 없으면 리뷰 후 지울까 고민입니다.

  • @NonRestartableComposable 어노테이션을 추가했습니다.

질문

질문 하나 있는데, material에서는 최소 클릭 영역을 보장하는 (각 48.dp) 확장함수를 구현해두었더라구요, 저희는 굳이 필요없을 것 같아서 안 넣었는데, 저희도 이런 확장함수가 필요할까요??

@Stable fun Modifier.minimumInteractiveComponentSize(): Modifier = this then MinimumInteractiveModifier

@kangyuri1114 kangyuri1114 added the enhancement New feature or request label Jul 28, 2024
@kangyuri1114 kangyuri1114 self-assigned this Jul 28, 2024
Comment on lines 80 to 88
* @param onClick Surface 클릭 시 실행되는 함수
* @param enabled Surface 클릭 가능 여부
* @param rounding Surface 모서리의 둥글기
* @param shape Surface 전체 모양. 기본값 : RectangleShape
* @param backgroundColor Surface 배경 색상. 기본값 : bgBasicDefault(#0xFFFFFFFF)
* @param contentColor Surface 내부 content 색상
* @param border Surface 테두리 굵기
* @param interactionSource Surface 상호작용 소스
* @param content Surface 내부 content
Copy link
Member

Choose a reason for hiding this comment

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

각 Surface마다 달라진 param들만 적어주는 것이 가독성에 좋을 것 같긴 하네용..!

Copy link
Member Author

Choose a reason for hiding this comment

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

음.. 사실 저도 처음에는 그렇게 생각했는데, 해당 주석들은 아래 사진처럼 배포 후 handy 사용 시에 어떤 함수들이고, 각 매개변수가 어떤 역할을 하는지 들어가보았을 때나 마우스를 가져다 놓았을 때 볼 수 있어요.
image

그런데 지금 4종류의 surface가 있는데 만약 clickable surface를 사용한다고 하면 그때마다 적혀있지 않은 매개변수의 역할을 보기 위해서 한 번 더 기본surface 함수를 찾아야 한다는 불편함이 있지 않을까 하는 고민이었는데......
어쩔까유.. 뭐가 좋을까요..?

Copy link
Member

@cometj03 cometj03 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
Member

Choose a reason for hiding this comment

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

Surface의 use-case를 작성하는 게 아니라면 없어도 될 것 같긴 하네요

**/

@Composable
@NonRestartableComposable
Copy link
Member

Choose a reason for hiding this comment

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

@NonRestartableComposable 어노테이션을 붙인 이유가 궁금합니다! 저도 확실하진 않지만 해당 어노테이션을 여기에 붙이는 게 조금 부적절해보여서요.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nonrestartablecomposable
위 링크 들어가보시면 해당 어노테이션에 대한 설명이 있는데요.. 정리하자면 성능을 위해서 불필요하게 해당 composable함수를 재실행하지 않기 위한 어노테이션 같아요.

그런데 material3에서 구현된 모든 surface에 붙어있더라구여..
사실 저도 왜.. surface에서 함수를 재실행하지 않도록 붙여야 하는거지.. 라는 의문이 있었는데,

제 조심스러운 추측으로surface composable 함수 자체가 다른 컴포넌트의 기반이 되는(레이아웃) 함수이기 때문에 변경 가능성이 적은 함수여서 그렇지 않을까 싶긴 합니다.. 저도 이걸 붙이는 게 좋을지 아닐지 많이 고민되는데 어떻게 하는 게 좋을까요

Copy link
Member

@cometj03 cometj03 Jul 30, 2024

Choose a reason for hiding this comment

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

아하 material3의 surface에서는 붙어있었군요. 흠 근데 일단은 관련해서 확실히 알고 있는 사람이 없다보니 일단은 제거하고, 나중에 확실히 성능 향상을 위해 붙여야겠다는 판단이 서면 그 때 붙이는 건 어떨까요?

지금은 있든 없든 크게 문제가 되진 않으니까요

저도 공부를 더 해봐야겠네욥

CompositionLocalProvider(
LocalContentColor provides contentColor,
) {
@Suppress("DEPRECATION_ERROR")
Copy link
Member

Choose a reason for hiding this comment

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

@Suppress("DEPRECATION_ERROR")의 의미가 무엇인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 어노테이션 자체는 deprecated된 api를 썼을 때 컴파일 에러를 막게 해주는 역할로 아는데, 요기에서는 필요없는 것 같아서 지우도록 하겠습니당~~

@kangyuri1114 kangyuri1114 merged commit 4f60802 into main Jul 30, 2024
@kangyuri1114 kangyuri1114 deleted the feature/rein/surface branch July 30, 2024 08:51
isContainer = true
}
.pointerInput(Unit) {},
propagateMinConstraints = true,

Choose a reason for hiding this comment

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

@kangyuri1114 propagateMinConstraints = true 일 필요가 있나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants