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: 유튜브 브랜딩 가이드에 따른 플레이리스트 추출 버튼 이미지 변경 #439

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

boogi-woogi
Copy link
Collaborator

관련 이슈번호

작업 사항

  • 플레이리스트 추출 버튼 이미지 변경

기타 사항

  • 플레이리스트로 추출할 음악이 하나도 선택되지 않은 경우 플레이리스트 추출 버튼을 클릭할 수 없다.

@boogi-woogi boogi-woogi added android 안드로이드 분야 feat 기능 개발 refactoring labels Nov 21, 2023
@boogi-woogi boogi-woogi self-assigned this Nov 21, 2023
@@ -20,7 +20,8 @@ sealed interface ScrapUiState {

data class Selection(
override val onSelect: (position: Int) -> Unit,
override val rooms: List<ScrappedRoomModel>
override val rooms: List<ScrappedRoomModel>,
val isAvailableExtraction: Boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

isAvailableExtraction -> isExtractionAvailable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영하겠습니다~

@@ -82,6 +82,13 @@ class ScrapListActivity : AppCompatActivity() {
SocialLogin.Google.getIntent(this)
)

is ScrapUiState.Selection -> {
binding.scrapIbExtractPlaylist.isEnabled = it.isAvailableExtraction
Copy link
Collaborator

Choose a reason for hiding this comment

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

isEnabled는 XML에서 반영 가능하지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UiState가 Selection 상태일때만 존재하는 값이라 Activity에서 수행했습니다~

Copy link
Collaborator

@whk06061 whk06061 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 12 to 14
val isSelected: Boolean
get() = _value.any { it.isSelected }

Copy link
Collaborator

Choose a reason for hiding this comment

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

변수 이름이 조금 더 직관적이면 좋을 것 같아요. 예를 들면 isAnySelected 이런 식? 좋은 변수명인지는 모르겠네영ㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hasBeenSelected라는 변수명으로 수정했는데 더 좋은게 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hasSelected 는 어떤가용 ( 선택된 걸 가지고 있냐? 느낌)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 좋네요~ 수정했습니다~

@boogi-woogi
Copy link
Collaborator Author

boogi-woogi commented Nov 22, 2023

isSelected -> hasBeenSelected로 수정했는데 흠.. 더 좋은게 생각이 안나네요

그리고 selected(선택되었는 지에 대한 상태)라는 것이 도메인에 존재해야하는 구조로 설계한 이유는 다음과 같습니다. 현재 플레이르스트로 추출할 때 선택된 룸이 10개로 제한되어 있는데 이것 역시 도메인 로직이라고 생각하고 해당 로직을 구현하기 위해서는 selected라는 사실을 도메인에서 알 필요가 있다고 생각합니다.
(추가적으로 스크랩된 룸에서 현재 선택된 룸만 뽑아내는 로직도 필요)

그리고 선택되었다라는 상태에 따라서 화면에 표현하는 방법이 제한적이지도 않기 때문에 뷰에 절대적으로 종속적인 로직도 아니라고 생각합니다~

@boogi-woogi boogi-woogi merged commit e953027 into android Nov 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android 안드로이드 분야 feat 기능 개발 refactoring
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants