단어 목록 화면 피그마 디자인에 맞게 변경 및 버그 수정

[easyhooon]

• 피그마 디자인 반영
• 단어 목록 화면에서 수정하기 버튼을 통해 단어 추가 화면으로 이동할 때 앱이 죽던 문제를 해결
• 뷰모델 내에 Context 참조 제거

[mwy3055]

참고: 브랜치를 잘못 지정한 경우에는 위의 edit 버튼을 눌러 target branch를 바꿀 수 있습니다. 귀찮게 PR을 닫고 다시 작성하지 않아도 됩니다.

[easyhooon]

참고: 브랜치를 잘못 지정한 경우에는 위의 edit 버튼을 눌러 target branch를 바꿀 수 있습니다. 귀찮게 PR을 닫고 다시 작성하지 않아도 됩니다.

default branch 가 develop 이 아니라 자꾸 실수하네요 ㅋㅋ edit 눌러봤는데 PR title 만 바꿀수있는줄알았는데 target 도 당연히(?) 바꿀 수 있나보군여 감사함니다

[mwy3055]

yes

[mwy3055]

0. 등록된 단어가 하나도 없을 때의 디자인을 추가했습니다. 홈 화면과 같은 내용을 보여주면 됩니다.

1. 초기화 버튼이 보이거나 사라질 때 애니메이션을 추가하면 어떨까요? AnimatedVisibility를 쓰고, 등장할 때는 fadeIn(), 사라질 때는 fadeOut()을 적용하면 될 것 같습니다.
   

2. ShowSearchOptionButton과 달리 SearchOptionClearButton에는 가운데에 4.dp짜리 Spacer가 없습니다. 추가 부탁드려요

3. 바텀 시트가 내려가 있을 때, 아래 그림에서 빨간 점선과 BottomNavigationBar 사이의 공간을 조금 넓힐 수 있을까요? 혹시 시트 높이가 고정되어 있나요?
   

4. WordClassChip 높이를 조금만 높일 수 있을까요? 한 1~2dp 정도면 될 것 같습니다. 피그마에서 디자인한 px 단위랑 안드로이드 dp가 정확히 1:1 대응하지는 않는 것 같아요
   

5. WordClassChip과 QuerySortState의 rounded corner 설정을 수정해 주세요. 기존에는 12.dp처럼 고정 값을 줬는데, RoundedCornerShape(percent = 50)처럼 비율로 줘야 할 것 같습니다. 피그마랑 안드로이드가 일대일 대응이 안 되네요. (border도 같은 shape 적용해야)

6. WordContent의 그림자를 아래로만 내릴 수는 없나요? 지금은 단어 카드가 약간 흐려 보이네요. 꼭 Card를 쓰지 않아도 될 것 같습니다.

7. 단어 맨 밑에, 빨간 점선을 기준으로 위아래 배경색이 다릅니다. colorScheme.surface로 통일해 주세요. 그리고 단어 콘텐츠 밑에 패딩을 조금 붙일 수 있을까요? 그림자가 부자연스럽게 끊기네요
   

8. 검색 버튼을 눌렀을 때 키보드가 내려가는 건 어떨까요? (제안입니다. 별로인 것 같으면 말씀해 주세요)

9. 모두 보기 화면에서 단어를 삭제한 후, 복원한 후, 같은 단어를 다시 삭제하면 스낵바가 뜨지 않습니다. 왜인가요? (Channel을 쓰면 해결할 수 있을까요?)

[mwy3055]

restoreWord는 무슨 역할을 하나요? 값이 UI에서 read되지 않고, ViewModel에서 write만 되고 있습니다.

[easyhooon]

1. 초기화 버튼이 보이거나 사라질 때 애니메이션을 추가하면 어떨까요? AnimatedVisibility를 쓰고, 등장할 때는 fadeIn(), 사라질 때는 fadeOut()을 적용하면 될 것 같습니다.

8. 검색 버튼을 눌렀을 때 키보드가 내려가는 건 어떨까요? (제안입니다. 별로인 것 같으면 말씀해 주세요)

좋네요~ 둘다 좋은 ui, ux 인거 같습니다~

[easyhooon]

3. 바텀 시트가 내려가 있을 때, 아래 그림에서 빨간 점선과 BottomNavigationBar 사이의 공간을 조금 넓힐 수 있을까요? 혹시 시트 높이가 고정되어 있나요?

어떻게든 해볼려고 짱구는 계속 굴려보는데 안되는거 같아서 일단 저렇게 PR을 올린거긴 합니다... 말씀하신 것 처럼 높이가 고정되어 있는 것 같아요(partiallyExpand state). 개같은 M3 BottomSheet

[easyhooon]

0, 1, 2, 4, 6, 7, 8 번을 코드 리뷰 반영 1 에서 수정했습니다

[easyhooon]

모두 보기 화면에서 단어를 삭제한 후, 복원한 후, 같은 단어를 다시 삭제하면 스낵바가 뜨지 않습니다. 왜인가요? (Channel을 쓰면 해결할 수 있을까요?)

one-time event 라 Channel 또는 SharedFlow 를 사용할 수 도 있는데 현재 코드 스타일이랑 많이 달라질 것 같아서 Boolean State 하나 더 추가하는 방식으로 Snackbar 가 같은 단어에 대해 한번만 호출되는 이슈 해결 에서 수정했습니다

restoreWord는 무슨 역할을 하나요? 값이 UI에서 read되지 않고, ViewModel에서 write만 되고 있습니다.

해당 로직과 관련된 문제였어서 UI 에서 사용되지 않는 restoreWord 파라미터 역시 Snackbar 가 같은 단어에 대해 한번만 호출되는 이슈 해결 에서 제거 하였습니다

[easyhooon]

6. WordContent의 그림자를 아래로만 내릴 수는 없나요? 지금은 단어 카드가 약간 흐려 보이네요. 꼭 Card를 쓰지 않아도 될 것 같습니다.

쉽지 않군요

[mwy3055]

수정하신 사항 확인했습니다. 읽다보니 든 생각인데, 수정 사항마다 커밋을 나누시는 게 좋았을 것 같습니다.

재수정사항 몇 가지 드립니다.

10. 단어와 뜻 사이가 원래 이렇게 좁았나요? 한 4dp 정도만 늘려주실 수 있으세요?
    EDIT: 현재 홈 화면과 모두 보기 화면에서 단어를 보여줄 때 사용하는 composable이 다릅니다. 모두 보기 쪽 composable로 통일할 수 있다면 좋을 것 같네요. (오른쪽 아이콘 부분만 lambda로 받게끔)
    

11. 얘는 왜 갑자기 위로 올라왔나요? 이전 PR에서는 가운데에 있었던 것으로 확인됩니다.
    

12. 단어 검색 화면에서, 검색 결과가 없을 때에도 단어를 추가하세요라는 메시지가 나옵니다. 디자인 실수입니다. ㅠ
    Figma에서 단어가 하나도 없는 경우와 단어가 있지만 검색 결과는 없는 경우 모두를 포함할 수 있는 디자인으로 수정했으니 확인 부탁드립니다. (위의 두 경우를 구분할 수도 있지만, 매우 귀찮아질 것 같습니다.)
    

13. 맨 처음 코멘트의 8번에서는 검색 버튼을 눌렀을 때 키보드가 내려가는 UX를 제안했으나, 지금은 팝업만 내려가고 있습니다. 키보드와 팝업 모두 내려가게 수정 부탁드려요.
    

[mwy3055]

참고로 Kotlin에서는 마지막 매개변수에도 trailing comma를 붙이는 것이 권장됩니다. 나중에 매개변수를 추가할 때 해당 라인을 변경하지 않아도 되기 때문입니다. (,가 사소하지만 은근히 눈에 띄는...)

권장 사항이기 때문에 변경 요청은 드리지 않습니다. 그냥 그렇다구요..

https://kotlinlang.org/docs/coding-conventions.html#trailing-commas

[easyhooon]

참고로 Kotlin에서는 마지막 매개변수에도 trailing comma를 붙이는 것이 권장됩니다. 나중에 매개변수를 추가할 때 해당 라인을 변경하지 않아도 되기 때문입니다. (,가 사소하지만 은근히 눈에 띄는...)

아 이거 저도 원래 다 붙혀주는 것을 고수하는데 현재 프로젝트에서는 trailing comma 를 붙이지 않는 설정(?)으로 되어 있길래, 이전 커밋에서 습관적으로 찍어주었던 trailing comma 들을 통일성을 위해 다 지운거긴 합니다 하하,,

[mwy3055]

엥 그런가요? 과거의 저를 원망해 주시길 바랍니다

[mwy3055]

리뷰하다 보니 인터랙션까지 피그마에 기술할 수 있었으면 하는 아쉬움이 드네요. ㅠㅠ

[easyhooon]

얘는 왜 갑자기 위로 올라왔나요? 이전 PR에서는 가운데에 있었던 것으로 확인됩니다.

이건 홈 화면 관련 건이긴 한데 반영하겠습니다. 또한 홈 화면꺼 merge 해서 충돌 해결 잘하면 제대로 반영되긴 할것 같슴다

[easyhooon]

단어와 뜻 사이가 원래 이렇게 좁았나요? 한 4dp 정도만 늘려주실 수 있으세요?
EDIT: 현재 홈 화면과 모두 보기 화면에서 단어를 보여줄 때 사용하는 composable이 다릅니다. 모두 보기 쪽 composable로 통일할 수 있다면 좋을 것 같네요. (오른쪽 아이콘 부분만 lambda로 받게끔)

이게 component 패키지의 wordContent 의 직접적인 수정을 가해서 그것을 사용하는 홈화면(TodayWord)에 영향이 간것으로 보입니다. wordContent 최대한 롤백하고 그것을 사용하는 측에서 컴포저블로 감싸든, modifier를 추가하는 등의 수정하는식으로 고칠게요!

[easyhooon]

11, 13번을 코드 리뷰 반영 2 에서 수정했습니다

[mwy3055]

11번, 13번 확인했습니다. 6번, 10번, 12번 남았네요.

그리고 변경 사항마다 커밋을 분리해 주세요. 리뷰어에게는 매우 중요한 사안입니다. ㅠㅠ

[easyhooon]

12번을 등록된 단어가 없는 경우와 검색 결과가 없는 경우를 분리 에서 수정했습니다

[easyhooon]

단어와 뜻 사이가 원래 이렇게 좁았나요? 한 4dp 정도만 늘려주실 수 있으세요?
EDIT: 현재 홈 화면과 모두 보기 화면에서 단어를 보여줄 때 사용하는 composable이 다릅니다. 모두 보기 쪽 composable로 통일할 수 있다면 좋을 것 같네요. (오른쪽 아이콘 부분만 lambda로 받게끔)

사이에 간격을 부여하는 작업은 가능하나, 모두 보기쪽 composable 로 통일하는 작업은 홈화면 작업 branch 와 많은 충돌이 발생할 것 같아 해당 작업은 홈화면 branch merge 후에 해야 할 듯합니다.

[mwy3055]

ok

마찬가지로 다음 PR에서 디테일을 채워주세요