[1단계 - 칸반 보드 생성(상품 목록)] 별터 미션 제출합니다.#2
[1단계 - 칸반 보드 생성(상품 목록)] 별터 미션 제출합니다.#2todays-sun-day wants to merge 23 commits intowoowacourse:todays-sun-dayfrom
Conversation
- 기존 코드 마이그레이션 여부 체크 - 기능요구 사항, 구현할 기능, 테스트 관련 내용 작성
- Kanban... 접두어 제거 및 Card로 통일
- Board 임시 구성 - Board 내 작성 버튼 구현 - Progress Bar 목업 구현 - Contents 출력용 Contents 섹션 구현
- 태그 입력이 올바르지 않을 경우 경고 문구 / 아이콘 출력 - 테스크 폼 유효성 검증에 따른 생성 버튼 활성화 로직 작성
| class CardData private constructor( | ||
| val title: String, | ||
| val content: String, | ||
| val tags: List<String>, |
There was a problem hiding this comment.
태그를 Tag 클래스와 Tags 클래스로 만들어서 CardData의 책임을 분리할까 고민했습니다.
하지만 다른 title이나 content 등도 별도의 클래스가 없이 분리를 안 했는데, Tag는 책임이 많다는 이유 하나로 분리해도 되는지가 고민이 되었습니다. 어떻게 하면 좋을지 리뷰어의 의견이 궁금합니다 !
There was a problem hiding this comment.
좋은 고민인데요. 한 가지 기준을 생각해보면 좋겠습니다.
현재 Tag 관련 로직은 파싱, 유효성 검사, 개수 제한, 길이 제한, 에러 메시지 등 여러 가지가 있잖아요. 반면 title은 빈 문자열 체크 정도고요.
만약 태그에 새로운 규칙이 추가된다면 (예: 특수문자 금지, 중복 태그 방지), 지금 구조에서는 어디를 수정해야 할까요? 수정해야 할 곳이 여러 군데라면, 그게 분리의 신호일 수 있습니다.
지금 당장 분리하지 않아도 괜찮지만, 별터만의 기준을 세워보시면 좋겠습니다.
| Column( | ||
| modifier = Modifier.padding(24.dp), | ||
| verticalArrangement = Arrangement.spacedBy(24.dp), | ||
| ) { | ||
| CardCreationPanelSection( | ||
| title = "제목 *", | ||
| placeholder = "태스크 제목을 입력하세요", | ||
| value = taskTitle, | ||
| onTextChange = { | ||
| taskTitle = it | ||
| }, | ||
| showAdditionalInfo = !CardData.isValidText(taskTitle), | ||
| infoText = CardData.getTitleInfo(), | ||
| isError = !CardData.isValidText(taskTitle), | ||
| ) | ||
|
|
||
| CardCreationPanelSection( | ||
| title = "설명", | ||
| placeholder = "태스크에 대한 자세한 설명을 입력하세요", | ||
| value = contents, | ||
| onTextChange = { contents = it }, | ||
| ) | ||
|
|
||
| CardCreationPanelSection( | ||
| title = "태그", | ||
| placeholder = "태그를 쉼표로 구분하여 입력하세요 (예: 버그, 긴급)", | ||
| value = tempTags, | ||
| onTextChange = { | ||
| tempTags = it | ||
| tagInfoText = CardData.isValidTagInfo(tempTags) | ||
| }, | ||
| showAdditionalInfo = true, | ||
| infoText = tagInfoText, | ||
| isError = !CardData.isValidTag(tempTags), | ||
| ) | ||
|
|
||
| CardCreationPanelStateSection( | ||
| selectedState = state, | ||
| onStateChange = { state = it }, | ||
| ) | ||
|
|
||
| CardCreationPanelManagerSection( | ||
| selectedManager = manager, | ||
| onManagerChange = { manager = it }, | ||
| ) |
There was a problem hiding this comment.
입력 필드 부분들을 따로 함수로 빼면 더 좋지 않았을까 생각합니다!
There was a problem hiding this comment.
CardCreationPanelSection으로 입력 필드를 컴포넌트화한 것은 좋은 접근이에요.
한 가지 더 생각해볼 포인트가 있는데요. 현재 CardCreationPanel이 모든 상태를 직접 관리하면서, 각 섹션에 상태를 전달하고 있잖아요. 만약 상태 관리와 UI 렌더링을 분리한다면 어떤 구조가 될까요?
'함수를 빼는 것'과 '책임을 분리하는 것'은 비슷하면서도 다른 이야기인데, 이 차이를 한 번 생각해보시면 좋겠습니다.
| StateButton( | ||
| text = "To Do", | ||
| isSelected = selectedState == "To Do", | ||
| onClick = { onStateChange("To Do") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), | ||
|
|
||
| ) | ||
| StateButton( | ||
| text = "In Progress", | ||
| isSelected = selectedState == "In Progress", | ||
| onClick = { onStateChange("In Progress") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), | ||
|
|
||
| ) | ||
| StateButton( | ||
| text = "Done", | ||
| isSelected = selectedState == "Done", | ||
| onClick = { onStateChange("Done") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), |
There was a problem hiding this comment.
상태를 현재 TextMatch로 확인하고 있는데, State 를 Enum 으로 관리하면 더 좋지 않을까 생각하고 있습니다. (밑의 관리자 버튼도 상태 버튼과 같은 코드 구조여서, 관리자도 Enum 으로 작성하는 것을 고려중에 있습니다.)
There was a problem hiding this comment.
이미 문제를 인식하고 계시네요. 좋은 방향이라고 생각합니다.
근데 sealed class 도 선택지에는 없었을까요? 만약 없었다면 학습을 해보고 이런 경우에 어떠한 것을 선택을 해야 하는지 혹은 이미 고려를 했는데 enum 을 선택한 것이라면 근거를 이야기 해주시면 좋겠습니다.
| @Test | ||
| fun `제목을 입력하지 않으면, 생성 버튼이 비활성화된다`() = runComposeUiTest { | ||
| // given | ||
| val blankTitle = "" | ||
|
|
||
| //when | ||
| setContent { | ||
| CardCreationPanel( | ||
| onAddItem = { CardData.create(blankTitle, "", listOf(("")), "구름") }, | ||
| onShowCardCreationPanel = {}, | ||
| ) | ||
| } | ||
|
|
||
| //then | ||
| onNodeWithText("생성").assertIsNotEnabled() | ||
| } |
There was a problem hiding this comment.
현재 onAddItem 의 텍스트들이 적절히 CreationPanel 창에 들어가서, 비활성화가 뜨는 건지 아니면 입력이 없다고 인식이 되어서 비활성화가 뜨는 건지 구분이 불명확하다고 생각합니다. 따라서, 활성화 조건일 때의 테스트를 추가하는 것이 좋다고 생각합니다.
There was a problem hiding this comment.
별터가 말씀하신 것처럼, 현재 테스트에서 비활성화 원인이 불명확한 게 맞습니다.
그렇다면 '활성화 조건일 때의 테스트'를 추가하려면 어떻게 해야 할까요? 지금 구조에서 '제목이 입력된 상태'를 테스트에서 만들 수 있을까요?
이 질문에 답을 찾다 보면, 앞서 이야기한 Stateful/Stateless 컴포넌트 구조와 연결될 거예요.
There was a problem hiding this comment.
@Test
fun `리컴포지션할 때 매번 유효성 검사`() = runComposeUiTest {
setContent {
Username(username = username, label = label)
}
username = "김컴포즈"
waitForIdle()
assertThat(validationCount).isEqualTo(1)
label = "바뀐 라벨"
waitForIdle()
assertThat(validationCount).isEqualTo(2)
}
위와 같은 코드처럼, 올바른 값을 입력했다가 틀린 값을 입력했다가 했을 시 리컴포지션이 잘 이루어지는지 확인하는 테스트 코드가 있으면 좋겠다고 생각합니다.
There was a problem hiding this comment.
리컴포지션과 유효성 검사의 관계를 인식하고 계신 점이 좋네요.
한 가지 생각해 볼 점이 있는데요. 리컴포지션이 "잘 이루어지는지"를 직접 검증하는 것보다, 리컴포지션의 "결과"를 검증하는 게 더 안정적인 테스트 아닐까요? 테스트로 검증하고자 하는 것 이 무엇인지를 다시 고민해 봅시다.
lee-ji-hoon
left a comment
There was a problem hiding this comment.
별터 안녕하세요! 코니입니다. 칸반 보드 생성 미션 빠르게 제출해 주셨네요.
제가 슬랙에 남겨드린 내용 중에 "마지막으로 빌드, 테스트 확인 후 리뷰 요청"라고 작성을 해두었는데, 실제로 테스트 코드 6건이 깨지고 있네요. 리뷰 요청 전에 테스트가 통과하는지 꼭 확인해 주세요.
그리고 Figma 기획서를 보면, 1단계는 새 태스크 생성 모달만 구현하는 단계입니다. 그런데 현재 보드 전체 레이아웃(프로그레스 바, 완료율 텍스트, 카드 리스트 등)까지 구현되어 있어요. 1단계 기획 범위를 벗어난 부분이고, 1단계 자체에서도 문제가 너무 많으니 1단계의 내용에만 우선 집중을 해보시기 바랍니다.
그리고
기획서에서 요구하는 범위를 정확히 파악하고,
그 범위 안에서 완성도 있게 구현하는 것이 중요합니다.
이번 리뷰에서는 1단계 범위를 벗어난 코드(Board, Card 등)는 의도적으로 리뷰하지 않았습니다. 2단계에서 git을 활용하여 커밋을 잘 분리하고, 해당 코드를 2단계 PR에 자연스럽게 포함하실 수 있게 해 보신다면 리뷰를 진행해 드리겠습니다.
그리고 1단계를 기준에서도 핵심 이슈 위주로 남겼고 일부 세부 사항은 패스했습니다. 핵심 이슈들부터 다 해결이 되면 추가적인 내용들을 리뷰를 하도록 하겠습니다. 추가로 궁금한 점이 있으면 채널에 말씀해 주세요!
| class CardData private constructor( | ||
| val title: String, | ||
| val content: String, | ||
| val tags: List<String>, |
There was a problem hiding this comment.
좋은 고민인데요. 한 가지 기준을 생각해보면 좋겠습니다.
현재 Tag 관련 로직은 파싱, 유효성 검사, 개수 제한, 길이 제한, 에러 메시지 등 여러 가지가 있잖아요. 반면 title은 빈 문자열 체크 정도고요.
만약 태그에 새로운 규칙이 추가된다면 (예: 특수문자 금지, 중복 태그 방지), 지금 구조에서는 어디를 수정해야 할까요? 수정해야 할 곳이 여러 군데라면, 그게 분리의 신호일 수 있습니다.
지금 당장 분리하지 않아도 괜찮지만, 별터만의 기준을 세워보시면 좋겠습니다.
| val tags: List<String>, | ||
| val manager: String, | ||
| ) { | ||
| companion object { |
There was a problem hiding this comment.
https://kotlinlang.org/docs/coding-conventions.html#class-layout 이 문서를 보고 위치를 다시 고민해보시기 바랍니다.
| fun isValidTag(rawText: String): Boolean { | ||
| if (rawText.isBlank()) return true | ||
|
|
||
| val parsedText = parseTag(rawText) | ||
| return (parsedText.all { isValidText(it) } && parsedText.size <= MAX_TAG_COUNT) | ||
| } |
There was a problem hiding this comment.
어떠한 정보는 팩토리에서 검증을 하고 어떤 것은 외부에 검증을 맡기고 있네요? 검증 은 누구의 역할일까요?
| val tags: List<String>, | ||
| val manager: String, | ||
| ) { | ||
| companion object { |
There was a problem hiding this comment.
- companion object 는 무엇인가요?
- companion object 는 어떠한 상황에 사용을 해야 하나요?
- companion object 는 java 기준으로 봤을 때 어떠한 역할을 하고 있을까요?
이 질문에 다 답변을 해주시면 되며, 해당 답변을 하다보면 지금 companion object 내부 코드들도 자연스럽게 변경이 될겁니다.
| import woowacourse.kanban.board.domain.CardData | ||
|
|
||
| @Composable | ||
| fun Board() { |
There was a problem hiding this comment.
1단계를 넘어가는 부분은 아예 리뷰 생략하겠습니다.
지금 무엇을 구현해야하는가. 를 구분하고 본인이 해야 할 작업을 먼저 정리하고 작업을 하는 것도 하나의 학습이기에 이번 일을 계기로 다음번엔 실수를 하지 않길 바랍니다.
| ManagerButton( | ||
| text = "다이노", | ||
| isSelected = selectedManager == "다이노", | ||
| onClick = { onManagerChange("다이노") }, | ||
| modifier = Modifier.width(200.dp).height(68.dp), | ||
| ) | ||
| ManagerButton( | ||
| text = "페임스", | ||
| isSelected = selectedManager == "페임스", | ||
| onClick = { onManagerChange("페임스") }, | ||
| modifier = Modifier.width(200.dp).height(68.dp), | ||
| ) |
There was a problem hiding this comment.
현재 상태 선택에서 selectedState == "To Do" 같은 문자열 비교를 사용하고 있는데요.
만약 여기서 "To Do"를 "ToDo"로 오타를 냈다면, 이 버그를 언제 발견할 수 있을까요? 컴파일 시점일까요, 런타임 시점일까요?
Kotlin에서 이렇게 고정된 선택지를 타입 안전하게 표현할 수 있는 방법이 있는데, 한 번 찾아보시면 좋겠습니다.
| fun isValidTag(rawText: String): Boolean { | ||
| if (rawText.isBlank()) return true | ||
|
|
||
| val parsedText = parseTag(rawText) | ||
| return (parsedText.all { isValidText(it) } && parsedText.size <= MAX_TAG_COUNT) | ||
| } |
There was a problem hiding this comment.
제가 실제로 크래시가 난 사례이고 https://github.com/woowacourse/compose-kanban-board-create/pull/2/changes#r2918544382 이 코멘트랑도 구조적으로 관련이 있으니 같이 해결하시기 바랍니다.
========================================
사용자가 태그에 "우아한테크코스"(7글자)를 입력하면, 생성 버튼이 활성화될까요? 그리고 실제로 생성 버튼을 누르면 어떤 일이 벌어질까요?
근본적으로 "에러 발생 예시" 피그마에서 지금 "생성" 버튼이 어떤 상황인지도 확인해보시기 바랍니다. 그리고 피그마가 아니더라도 이런 기본적인 UX는 지켜야 합니다. 에러 상태인데 성공 생성 같은 버튼이 활성화가 되어서는 안됩니다.
| StateButton( | ||
| text = "To Do", | ||
| isSelected = selectedState == "To Do", | ||
| onClick = { onStateChange("To Do") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), | ||
|
|
||
| ) | ||
| StateButton( | ||
| text = "In Progress", | ||
| isSelected = selectedState == "In Progress", | ||
| onClick = { onStateChange("In Progress") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), | ||
|
|
||
| ) | ||
| StateButton( | ||
| text = "Done", | ||
| isSelected = selectedState == "Done", | ||
| onClick = { onStateChange("Done") }, | ||
| modifier = Modifier.width(200.dp).height(52.dp), |
There was a problem hiding this comment.
이미 문제를 인식하고 계시네요. 좋은 방향이라고 생각합니다.
근데 sealed class 도 선택지에는 없었을까요? 만약 없었다면 학습을 해보고 이런 경우에 어떠한 것을 선택을 해야 하는지 혹은 이미 고려를 했는데 enum 을 선택한 것이라면 근거를 이야기 해주시면 좋겠습니다.
| @Test | ||
| fun `제목을 입력하지 않으면, 생성 버튼이 비활성화된다`() = runComposeUiTest { | ||
| // given | ||
| val blankTitle = "" | ||
|
|
||
| //when | ||
| setContent { | ||
| CardCreationPanel( | ||
| onAddItem = { CardData.create(blankTitle, "", listOf(("")), "구름") }, | ||
| onShowCardCreationPanel = {}, | ||
| ) | ||
| } | ||
|
|
||
| //then | ||
| onNodeWithText("생성").assertIsNotEnabled() | ||
| } |
There was a problem hiding this comment.
별터가 말씀하신 것처럼, 현재 테스트에서 비활성화 원인이 불명확한 게 맞습니다.
그렇다면 '활성화 조건일 때의 테스트'를 추가하려면 어떻게 해야 할까요? 지금 구조에서 '제목이 입력된 상태'를 테스트에서 만들 수 있을까요?
이 질문에 답을 찾다 보면, 앞서 이야기한 Stateful/Stateless 컴포넌트 구조와 연결될 거예요.
| @Test | ||
| fun `리컴포지션할 때 매번 유효성 검사`() = runComposeUiTest { | ||
| setContent { | ||
| Username(username = username, label = label) | ||
| } | ||
|
|
||
| username = "김컴포즈" | ||
| waitForIdle() | ||
| assertThat(validationCount).isEqualTo(1) | ||
|
|
||
| label = "바뀐 라벨" | ||
| waitForIdle() | ||
| assertThat(validationCount).isEqualTo(2) | ||
| } |

Background
Test
default.mp4