Implement lecture search/#10 #46

gahyuun · 2024-03-08T02:27:45Z

📌 작업 내용

구현 내용 및 작업 했던 내역

커스텀 버튼 클릭 시 과목 검색 컴포넌트 렌더링
추가 버튼 클릭 시 기이수 과목 리스트에 반영

이후 검색 기능 구현과 커스텀 저장 기능을 차례대로 할 예정입니다!

text-input을 사용하는데 font size인 text-sm이 너무 작다고 느껴집니다 이 부분 variant를 통해 조정해도 괜찮을지 여쭤보고싶습니다
select 컴포넌트의 min width를 10rem으로 설정하셨는데 그것보다 더 작게 하고 싶어 min width 속성을 제거했습니다 혹시 꼭 필요한 속성이라면 말씀해주세요 변경하겠습니다

🤔 고민 했던 부분

과목 검색 컴포넌트 ui를 지금 졸부 서비스와 다르게 가져갔습니다 이유는 다음과 같습니다

버튼 클릭을 통해 과목명, 과목코드 키워드를 변경하는 것이 사용자에게 어렵게 느껴질 거라고 판단해서 select로 변경했습니다 저는 처음 봤을 때 해당 기능이 어떤 역할을 하는 지 한 번에 파악하기 힘들었습니다
검색하기 버튼이 있으면 디자인이 안예쁘다고 생각해서 제거했습니다. 꼭 검색하기 버튼이 필요한 이유도 모르겠더라구요

🔊 도움이 필요한 부분

LectureSearchResultContainer 에서 addLecture 함수를 구현하지 않고 TakenLectureList(부모 컴포넌트)에서 addLecture을 구현하고 props를 통해 전달했습니다

LectureSearchResultContainer 함수의 역할을 강의 검색 결과를 렌더링해주는 컴포넌트, TakenLectureList 함수의 역할을 커스텀을 할 수 있는 기이수 과목 리스트라고 정의를 내렸습니다. 그러면 강의를 추가해주는 역할은 TakenLectureList라고 생각했습니다
LectureSearchResultContainer에는 강의 검색에만 의존하고 커스텀에 대한 의존성을 제거하려했습니다

네이밍 같은 경우는 이때했던 논의를 참고했습니다
#42 (comment)

제가 진행 한 컴포넌트 분리가 관심사를 잘 분리한 것인지 확신이 안드네요 다른 분들 의견 더 주시면 감사하겠습니다!

❗️리팩토링

논의를 기반으로 리팩토링을 진행했습니다

taken lecture list에는 list의 역할을 하는 것이므로 Lecture search 까지 렌더링할 필요가 없다고 생각했고 그래서 LectureSearch 컴포넌트를 taken lecture list 컴포넌트 외부에
서 렌더링 시켰습니다

github-actions · 2024-03-08T02:29:24Z

🚀storybook: https://65ccb85d5afe55a024495bc0-axtyjslool.chromatic.com/

yougyung

수고하셨습니다 👍

yougyung · 2024-03-08T09:56:12Z

app/ui/view/molecule/list/list-root.tsx

  return (
-    <div className="rounded-2xl border-[1px] border-black-2 w-full">
+    <div className={twMerge('rounded-xl border-[1px] border-gray-300 w-full ', isScrollList && 'h-72 overflow-auto')}>


isScrollList && 'h-72 overflow-auto 위 식이 거짓인 경우에는 className에 false값도 들어가더라고요! 저도 최근에 알게 된 점이라서 &&논리연산자를 사용하고 싶을때는 cn연산자를 사용합니다.

cn util함수는 shadcn설정을 진행하면서 app>utils/shadcn/utils.ts 내부에 이미 위치해져있으며, cn함수가 twMerge와 clsx를 조합한 것이기 때문에 false를 filtering하는 것도 가능합니다 :)

Suggested change

<div className={twMerge('rounded-xl border-[1px] border-gray-300 w-full ', isScrollList && 'h-72 overflow-auto')}>

<div className={cn('rounded-xl border-[1px] border-gray-300 w-full ', isScrollList && 'h-72 overflow-auto')}>

감사합니다 수정했습니다 0a9c473

yougyung · 2024-03-08T09:59:39Z

app/type/lecture.ts

@@ -1,8 +1,17 @@
-export type LectureInfo = {
+export interface LectureInfo {
+  [index: string]: string | number;


[index: string]: string | number; 어떻게 사용되나요?

object.keys 메서드를 사용할 때 사용하는 key는 string 타입인데, 이 string 타입을 string literal type에 할당하려 했기 때문에 오류가 발생했었고 위 구문을 통해 해결했습니다

yougyung · 2024-03-08T10:15:56Z

app/ui/lecture/lecture-search/index.tsx

해당 컴포넌트가 도메인 컴포넌트에 위치하는 가현이의 생각이 궁금해요 !

해당 컴포넌트는 리팩토링을 통해 변경되었습니다!

github-actions · 2024-03-12T13:28:20Z

🚀storybook: https://65ccb85d5afe55a024495bc0-kynwwdpktn.chromatic.com/

yougyung

고민의 흔적이 느껴지는 PR이네요. 수고하셨습니다 👍

text-input을 사용하는데 font size인 text-sm이 너무 작다고 느껴집니다 이 부분 variant를 통해 조정해도 괜찮을지 여쭤보고싶습니다

동일하게 개발을 진행하면서 불편을 느꼈던 부분입니다! 한 명이 담당해서 전체적인 button크기에 대한 refactoring을 진행하는 것이 좋다고 생각이 듭니다.

검색하기 버튼이 있으면 디자인이 안예쁘다고 생각해서 제거했습니다. 꼭 검색하기 버튼이 필요한 이유도 모르겠더라구요

말씀하신 의견과 같이 검색하기 버튼이 없는 것이 UI/UX측면에서 유리하다고 생각이 듭니다. 이를 위해서는 debouncing 및 enter를 통한 api호출만 반영되어야 할 것 같은데, 두가지 기능 모두 계획하고 계신지 궁급합니다!

gahyuun · 2024-03-13T00:31:03Z

고민의 흔적이 느껴지는 PR이네요. 수고하셨습니다 👍

text-input을 사용하는데 font size인 text-sm이 너무 작다고 느껴집니다 이 부분 variant를 통해 조정해도 괜찮을지 여쭤보고싶습니다

동일하게 개발을 진행하면서 불편을 느꼈던 부분입니다! 한 명이 담당해서 전체적인 button크기에 대한 refactoring을 진행하는 것이 좋다고 생각이 듭니다.

검색하기 버튼이 있으면 디자인이 안예쁘다고 생각해서 제거했습니다. 꼭 검색하기 버튼이 필요한 이유도 모르겠더라구요

말씀하신 의견과 같이 검색하기 버튼이 없는 것이 UI/UX측면에서 유리하다고 생각이 듭니다. 이를 위해서는 debouncing 및 enter를 통한 api호출만 반영되어야 할 것 같은데, 두가지 기능 모두 계획하고 계신지 궁급합니다!

넹 추후에 두 가지 기능 반영 할 예정입니다!
button 크기는 언니가 리팩토링하는 게 나을까요!?

seonghunYang

수고하셨습니다.
보면서 느낀점은, 이번 리팩토링을 통한 가장 큰 개선점은 가독성이라고 생각이 들어요. 각 컴포넌트가 어떤 책임을 수행하는지 코드를 읽는 사람이 크게 고민할 필요 없이 쉽게 이해할 수 있었습니다.
이런 구조라면 변경사항이 생겼을 때도, 코드를 이해하고 변경해야 할 위치를 쉽게 찾을 수 있을 것 같습니다.
한 가지 걸리는 점이 있다면, taken-lecture 컴포넌트에서 lecture-search 컴포넌트를 호출하는 것이네요. 동일한 수준의 컴포넌트를 호출하는 것은 큰 문제가 되지 않지만, 이 두 컴포넌트는 더 상위 수준의 컴포넌트인 페이지 컴포넌트(컨테이너)에서 결합하는 것이 더 적합해 보입니다.
버튼 크기는 효율성을 위해 유경이가 리팩토링하면 될 것 같네요. 미리 고마워요

seonghunYang · 2024-03-08T09:35:24Z

app/__test__/ui/lecture/taken-lecture-list.test.tsx

    const customButton = await screen.findByTestId('custom-button');
    await userEvent.click(customButton);


[comment]

이렇게 2줄로 나눠서 작성한 이유가 있을까요?

저는 한 줄에 작성한 것보다 두 줄에 나눠서 작성하는 게 작업의 흐름이 더 잘 읽힌다고 생각해서 그렇게 작업했습니다!

gahyuun · 2024-03-13T07:29:26Z

수고하셨습니다.

보면서 느낀점은, 이번 리팩토링을 통한 가장 큰 개선점은 가독성이라고 생각이 들어요. 각 컴포넌트가 어떤 책임을 수행하는지 코드를 읽는 사람이 크게 고민할 필요 없이 쉽게 이해할 수 있었습니다.

이런 구조라면 변경사항이 생겼을 때도, 코드를 이해하고 변경해야 할 위치를 쉽게 찾을 수 있을 것 같습니다.

한 가지 걸리는 점이 있다면, taken-lecture 컴포넌트에서 lecture-search 컴포넌트를 호출하는 것이네요. 동일한 수준의 컴포넌트를 호출하는 것은 큰 문제가 되지 않지만, 이 두 컴포넌트는 더 상위 수준의 컴포넌트인 페이지 컴포넌트(컨테이너)에서 결합하는 것이 더 적합해 보입니다.

버튼 크기는 효율성을 위해 유경이가 리팩토링하면 될 것 같네요. 미리 고마워요

그렇네요! 컨테이너에서 결합하는 게 더 좋을 것 같아서 수정했습니다 감사합니다:)
ccf604c

github-actions · 2024-03-13T07:31:06Z

🚀storybook: https://65ccb85d5afe55a024495bc0-iichcqsgup.chromatic.com/

seonghunYang

확인했습니다.
테스트 코드에서 render() 함수가 두 번 사용되는 것은 조금 일반적이지 않다고 생각이 들긴 했으나, 서버 컴포넌트가 아직 지원되지 않아서 특별한 방식으로 사용되는 상황이기 때문에 어쩔 수 없다는 생각이 듭니다.

gahyuun added 11 commits March 6, 2024 23:48

feat: add radix ui icon package

e73e216

fix: solve index signature error

62c96e6

style: change list row text color

06e0d9e

chore: add search result icon

6222592

style: delete min width styling

559e166

feat: implement add lecture

07346d9

feat: implement lecture search bar

deb4de2

feat: implement lecture search result container

5f39f00

feat: implement lecture search

79dd277

refactor: seperate searched lecture info type

e81447e

test: implement lecture search rendering test code

bc330c0

gahyuun self-assigned this Mar 8, 2024

gahyuun requested review from yougyung and seonghunYang as code owners March 8, 2024 02:27

yougyung previously approved these changes Mar 8, 2024

View reviewed changes

gahyuun added 6 commits March 12, 2024 15:46

feat: add jotai package

1cecdeb

feat: implement custom taken lecture store

88d1a88

refactor: improve lecture search result container

0a84620

refactor: improve take

7361b74

refactor: improve taken lecture label

9ac053e

refactor: improve lecture search

5611eb6

gahyuun dismissed yougyung’s stale review via 5611eb6 March 12, 2024 06:50

gahyuun added 3 commits March 12, 2024 15:54

chore: delete import

c5af991

refactor: change twMerge -> cn

0a9c473

chore: merge branch 'main'

b9f5106

yougyung previously approved these changes Mar 12, 2024

View reviewed changes

seonghunYang reviewed Mar 13, 2024

View reviewed changes

refactor: delete lecture search

ccf604c

gahyuun dismissed yougyung’s stale review via ccf604c March 13, 2024 07:29

yougyung approved these changes Mar 14, 2024

View reviewed changes

seonghunYang approved these changes Mar 14, 2024

View reviewed changes

gahyuun merged commit 8fbd295 into main Mar 14, 2024
3 checks passed

gahyuun deleted the implement-lecture-search/#10 branch March 14, 2024 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lecture search/#10 #46

Implement lecture search/#10 #46

gahyuun commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024

yougyung left a comment

yougyung Mar 8, 2024

gahyuun Mar 12, 2024

yougyung Mar 8, 2024

gahyuun Mar 12, 2024

yougyung Mar 8, 2024

gahyuun Mar 12, 2024

github-actions bot commented Mar 12, 2024

yougyung left a comment •

edited

Loading

gahyuun commented Mar 13, 2024 •

edited

Loading

seonghunYang left a comment

seonghunYang Mar 8, 2024

gahyuun Mar 13, 2024

gahyuun commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

seonghunYang left a comment

	<div className={twMerge('rounded-xl border-[1px] border-gray-300 w-full ', isScrollList && 'h-72 overflow-auto')}>
	<div className={cn('rounded-xl border-[1px] border-gray-300 w-full ', isScrollList && 'h-72 overflow-auto')}>

		const customButton = await screen.findByTestId('custom-button');
		await userEvent.click(customButton);

Implement lecture search/#10 #46

Implement lecture search/#10 #46

Conversation

gahyuun commented Mar 8, 2024 • edited Loading

📌 작업 내용

🤔 고민 했던 부분

🔊 도움이 필요한 부분

❗️리팩토링

github-actions bot commented Mar 8, 2024

yougyung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 12, 2024

yougyung left a comment • edited Loading

Choose a reason for hiding this comment

gahyuun commented Mar 13, 2024 • edited Loading

seonghunYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gahyuun commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

seonghunYang left a comment

Choose a reason for hiding this comment

gahyuun commented Mar 8, 2024 •

edited

Loading

yougyung left a comment •

edited

Loading

gahyuun commented Mar 13, 2024 •

edited

Loading