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

[YDS-#262] YDS Icon에 contentDescription 속성 추가 #263

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

kangyuri1114
Copy link
Member

Summary

  • Soomsil V2 홈: Collapsible Search Bar 구현 중 필요 요청에 의하여 추가하였습니다.

Describe your changes

  • Material3의 Icon 구현 코드를 참고하였습니다. (Modifier.semantics 활용)
  • versions.properties 2.5.6 -> 2.5.7 로 업데이트

Issue

To reviewers

  • 자동 배포를 위해 우선 versions.properties를 업데이트 해야하는 것 같아 2.5.7로 올려서 push하긴 했는데, 이전 pr을 보니 수동 배포를 생각 중이신 것 같은데 어떻게 해야 할까요..!
  • Material3의 Icon은 contentDescription에 default 값이 없던데, YDS에는 그렇게 하면 Icon을 사용하는 모든 코드들이 수정될 것 같아 default로 null을 두어서 기존에 Icon을 사용하는 코드들이 수정되지 않고 선택적으로 contentDescription을 쓸 수 있도록 했습니다.

PR 올리기 전 체크 리스트

  • version.properties의 버전을 업데이트 했나요?

@kangyuri1114 kangyuri1114 self-assigned this Mar 17, 2024
@kangyuri1114 kangyuri1114 requested a review from a team March 17, 2024 09:21
@kangyuri1114 kangyuri1114 added the enhancement New feature or request label Mar 17, 2024
cometj03
cometj03 previously approved these changes Mar 17, 2024
Copy link
Contributor

@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.

👍 👍

라이브러리 배포는 develop 브랜치에 push하면 자동으로 됩니다!

)
}

@Composable
fun Icon(
imageVector: ImageVector,
contentDescription: String?,
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 기본값을 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.

YDS 코드 내에서는 imageVector: ImageVector을 매개변수로 가지는 해당 Icon을 직접적으로 사용하는 곳이 없어서 null을 넣어둘까 고민하다가 넣지 않긴 했는데, 그냥 넣어두어도 될까요?!

어차피 @DrawableRes id: Int을 매개변수로 가지는 위쪽 Icon에 null이 들어가면 이 Icon에도 null이 들어갈 것이라고 생각했습니당..!

@kangyuri1114 kangyuri1114 merged commit dac24b8 into develop Mar 20, 2024
1 check passed
@kangyuri1114 kangyuri1114 deleted the feature/rein/icon-contentDescription branch March 20, 2024 03:16
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.

Icon contenDescription 추가
2 participants