[4주차-TS] 🎤노인코래방🎤 by leeseooo · Pull Request #7 · THE-SOPT-WEB/leeSeoYeong

leeseooo · 2022-06-24T15:30:09Z

✨ 구현 기능 명세

지역기반, 동네기반 노래방 검색

🎁 PR Point

질문있어요

자동으로 타입 추론이 되면 굳이 타입을 명시해주지 않아도 괜찮은지 궁금합니다!
components 폴더의 컴포넌트들의 네이밍이 혹시 어색하지 않은지 궁금해요! 검색하는 영역, 결과를 보여주는 영역이라는 의미로 ~Section이라고 지었긴 한데 작명이 제일 어렵더라고요?
ResultSection.tsx 컴포넌트에서 api요청 후 검색결과 출력할 때 밑에 두 방법 중에서 뭐가 더 나을까요? 아니면 얘네보다 더 좋은 다른 표현 방법이 있을까요?

삼항연산자로 표현

{!isLoading && storeInfo.length === 0 ? (
        <div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>
      ) : (
        storeInfo?.map((store: Store, idx: number) => (
          <StoreCard key={idx} store={store} isCheck={isCheck} />
        ))
      )}

각각 &&로 표현 -> !isLoading이 중복됨

 {!isLoading && storeInfo.length === 0 && (
        <div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>
      )}
{!isLoading &&
        storeInfo?.map((store: Store, idx: number) => (
          <StoreCard key={idx} store={store} isCheck={isCheck} />
))}

😭 어려웠던 점

😎 구현 결과물

Vite.App.-.Chrome.2022-06-25.00-26-29_Trim.mp4

github-actions · 2022-06-24T15:30:32Z

오늘도 할할놀놀을 몸소 실천 중인 웹파트원 ! 화이팅 :)
과제 레퍼런스 참고해서 빠트린 부분은 없는 지 체크해볼까요?

joohaem

멋지네요 !!!!! 후에 제대로 리뷰하겠씁니다!

Nahee-Park

멋져여 !!!!! 스켈레톤에 ts까지 야무지게 잘 쓰신 것 같습미다 !!!!

자동으로 타입 추론이 되면 굳이 타입을 명시해주지 않아도 괜찮은지 궁금합니다!

다른 언어들과는 달리 타입스크립트는 타입 추론이 되도록 유도하고, 타입이 추론되는 경우 굳이 타입을 명시해주지 않는 것이 컨벤션인 것 같습니다. 그런데 저도 이 부분에 대해 명확하게 왜 그렇게 하는 지 이유를 잘 모르겠습니다. 왜냐하면 타언어에선 타입이 추론되는 원시타입들도

코드에 명시적으로 타입을 나타내기 위해 (사람을 위해)
컴파일 단계에서 불필요한 타입 추론 단계를 줄이기 위해 (컴퓨터를 위해)

위와 같은 이유들로 항상 타입을 명시할 것을 권장하고 있는 경우들이 많더라구요. 어떠한 이유로 타입스크립트에서는 타입 추론이 컨벤션이 된 것인지는 .. 저도 생각을 나눠보고 싶은 주제인 것 같아요

components 폴더의 컴포넌트들의 네이밍이 혹시 어색하지 않은지 궁금해요! 검색하는 영역, 결과를 보여주는 영역이라는 의미로 ~Section이라고 지었긴 한데 작명이 제일 어렵더라고요?

이건 .. 항상 정말 어렵고 개인 취향도 반영되고 컨벤션도 반영되는 영역인 것 같아요. 근데 현재 제 개인적인 생각으로 어색한 컴포넌트 명은 없는 것 같습니다 !

ResultSection.tsx 컴포넌트에서 api요청 후 검색결과 출력할 때 밑에 두 방법 중에서 뭐가 더 나을까요? 아니면 얘네보다 더 좋은 다른 표현 방법이 있을까요?

이 부분 리뷰에도 정리해보았는데 가독성을 위해 중첩 삼항연산자를 지양하는 방식이 좋은 것 같고, 혹은 조건부 렌더링을 고려해보는 것도 좋은 것 같습니다!

Nahee-Park · 2022-06-25T06:20:17Z

+      {isLoading &&
+        new Array(10).fill(1).map((_, idx) => {
+          return <Skeleton key={idx} />;
+        })}
+      {!isLoading && storeInfo.length === 0 ? (
+        <div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>
+      ) : (
+        storeInfo?.map((store: Store, idx: number) => (
+          <StoreCard key={idx} store={store} isCheck={isCheck} />
+        ))
+      )}


오 개인적으로 중첩 삼항연산자보다 &&와 삼항연산자를 함께 쓴 이 방식이 확실히 좀 더 가독성 좋은 방식인 거 같아요!

보편적으로 많이 쓰이는 eslint airbnb js 스타일 가이드에 아래와 같은 룰이 있는데 한 번 읽어봐도 좋을 것 같아요! (물론 이 가이드는 절대 정답이 아니고 협업할 때마다 다른 lint룰을 사용하게 될 거라 정말 참고용입미다 ! )

https://github.com/parksb/javascript-style-guide

참고하겠습니다 고마워요!!

Nahee-Park · 2022-06-25T06:30:08Z

+      {isLoading &&
+        new Array(10).fill(1).map((_, idx) => {
+          return <Skeleton key={idx} />;
+        })}
+      {!isLoading && storeInfo.length === 0 ? (
+        <div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>
+      ) : (
+        storeInfo?.map((store: Store, idx: number) => (
+          <StoreCard key={idx} store={store} isCheck={isCheck} />
+        ))
+      )}


Suggested change

{isLoading &&

new Array(10).fill(1).map((_, idx) => {

return <Skeleton key={idx} />;

})}

{!isLoading && storeInfo.length === 0 ? (

<div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>

) : (

storeInfo?.map((store: Store, idx: number) => (

<StoreCard key={idx} store={store} isCheck={isCheck} />

))

)}

if(isLoading) {

new Array(10).fill(1).map((_, idx) => {

return <Skeleton key={idx} />;

})

} else {

return (

<>

{storeInfo.length === 0 ? (

<div style={{ color: "#fff" }}>검색 결과가 없습니다.</div>

) : (

storeInfo?.map((store: Store, idx: number) => (

<StoreCard key={idx} store={store} isCheck={isCheck} />

))

)}

</>

)

}

혹은 이런 식으로 조건부 렌더링을 하는 방식도 있을 것 같아여 !

혹은 나중에 서버사이드 데이터 처리 상태를 로딩 상태 뿐만 아니라 에러 상태까지 확장시킨다면 이런 식으로 switch-case문을 이용한 조건부렌더링을 할 수도 있을 것 같아요 !

function Result({ isEmpty, keywordLocationData, status }: ResultProps) { switch (status) { case 'LOADING': return <Loading />; case 'ERROR': return <ErrorFallback />; default: return isEmpty ? <NoResult /> : <CardList keywordLocationData={keywordLocationData} />; } }

근데 사실 이거 다 갠취인 영역이라 정답은 없어서 갠취대로 갈기면 될 거 같긴 해여 ㅋ ㅋ

아 저렇게 하면 되는구나 ㅋㅋ저런식으로 갈겨볼게여
switch문 상태 처리 코드도 참고할게요 감삼닷!

Nahee-Park · 2022-06-26T06:07:23Z

+  const [isCheck, setIsCheck] = useState<boolean>(false);
+  const [myTown, setMyTown] = useState<string>("");


state로 input value를 관리하고 있다면 useInput같은 커스텀 훅을 정의해서 사용해도 좋을 것 같아요!

커스텀 훅에 관해서 https://react.vlpt.us/basic/21-custom-hook.html 이런 자료들 보면 좋을 것 같아여 !

이번 기회에 커스텀 훅을 한 번 시도해볼게여

Nahee-Park · 2022-06-26T06:19:23Z

+  background-color: #bdbdbd;
+`;
+
+export default Skeleton;


크. .. .스켈레톤 멋있어여

데브스톤이 더 멋있어여

Nahee-Park · 2022-06-26T06:35:18Z

+  return (
+    <StWrapper>
+      <StCheckBoxWrapper>
+        <label htmlFor="checkbox">지역 기반으로 검색</label>
+        <input
+          type="checkbox"
+          id="checkbox"
+          onChange={() => setIsCheck(!isCheck)}
+        />
+      </StCheckBoxWrapper>
+
+      <StTextInputWrapper>
+        <label htmlFor="text">우리 동네는 여기에요</label>
+        <input
+          type="text"
+          placeholder="지역을 입력해주세요."
+          disabled={isCheck}
+          value={myTown}
+          onChange={(e) => setMyTown(e.target.value)}
+        />
+      </StTextInputWrapper>
+
+      <StSearchButton type="button" onClick={() => onClick(isCheck, myTown)}>
+        검색하기
+      </StSearchButton>
+    </StWrapper>
+  );
+}
+
+const StWrapper = styled.div`
+  display: flex;
+  flex-direction: column;
+  justify-content: center;
+  align-items: center;
+  padding: 10px 0;
+  gap: 10px;
+  font-size: 20px;
+  color: #fff;
+  border-bottom: 2px solid #fff;
+  width: 100%;
+`;


이 부분 form테그로 wrapping해서 사용하면 좋을 것 같아요!

Suggested change

return (

<StWrapper>

<StCheckBoxWrapper>

<label htmlFor="checkbox">지역 기반으로 검색</label>

<input

type="checkbox"

id="checkbox"

onChange={() => setIsCheck(!isCheck)}

/>

</StCheckBoxWrapper>

<StTextInputWrapper>

<label htmlFor="text">우리 동네는 여기에요</label>

<input

type="text"

placeholder="지역을 입력해주세요."

disabled={isCheck}

value={myTown}

onChange={(e) => setMyTown(e.target.value)}

/>

</StTextInputWrapper>

<StSearchButton type="button" onClick={() => onClick(isCheck, myTown)}>

검색하기

</StSearchButton>

</StWrapper>

);

}

const StWrapper = styled.div`

display: flex;

flex-direction: column;

justify-content: center;

align-items: center;

padding: 10px 0;

gap: 10px;

font-size: 20px;

color: #fff;

border-bottom: 2px solid #fff;

width: 100%;

`;

return (

<StForm onSubmit={(e) => onClick(isCheck, myTown, e)}>

<StCheckBoxWrapper>

<label htmlFor="checkbox">지역 기반으로 검색</label>

<input

type="checkbox"

id="checkbox"

onChange={() => setIsCheck(!isCheck)}

/>

</StCheckBoxWrapper>

<StTextInputWrapper>

<label htmlFor="text">우리 동네는 여기에요</label>

<input

type="text"

placeholder="지역을 입력해주세요."

disabled={isCheck}

value={myTown}

onChange={(e) => setMyTown(e.target.value)}

/>

</StTextInputWrapper>

<StSearchButton type="submit">

검색하기

</StSearchButton>

</StForm>

);

}

const StForm = styled.form`

display: flex;

flex-direction: column;

justify-content: center;

align-items: center;

padding: 10px 0;

gap: 10px;

font-size: 20px;

color: #fff;

border-bottom: 2px solid #fff;

width: 100%;

`;

근데 form tag를 쓰고 onSubmit이벤트를 발생시키면 디폴트로 브라우저에서 새로고침하는 동작을 넣어버려서 onSubmit이벤트 내부에 들어있는 함수에서 그런 불필요한 디폴트 동작을 막아줘야 하는데 (SPA에서 새로고침은 불필요한 동작이니까여 !) 그러기 위해서

일단 event인자를 받아주도록 수정하고,

아 7주차 과제에서 이런 식으로 짰었는데 이것도 그렇게 바꿔볼게요!
이 방법이 더 좋아보이네요

Nahee-Park · 2022-06-26T06:44:21Z

+  const [isChecked, setIsChecked] = useState<boolean>(false);
+  const [isLoading, setIsLoading] = useState(false);
+
+  const onClickSearchButton = (isCheck: boolean, myTown: string) => {


브라우저가 새로고침되는 이벤트 디폴트 동작을 막기 위해

Suggested change

const onClickSearchButton = (isCheck: boolean, myTown: string) => {

const onClickSearchButton = (isCheck: boolean, myTown: string, e:React.FormEvent<HTMLFormElement>) => {

e.preventDefault();

이런 식으로 event를 받아서 e.preventDefault();를 통해 디폴트 이벤트를 막아주면 좋을 것 같습미다!

joohaem

개자래이서영😣👍🎆🍉

결과를 보여주는 영역이라는 의미로 ~Section이라고 지었긴 한데

나도 그렇게 지었던 것 같은데 나 같은 경우에는

해당 페이지에 3개의 section만 존재하여 위계가 같음
각각 시멘틱 태그 section을 사용할 것임

이런 이유로 그렇게 네이밍 했던 것 같아!

1, 3번 부분은 나히가 설명 잘 해준 것 같네요!!!! 배워갑미다....나커박🤩🤩 @Nahee-Park

joohaem · 2022-06-26T10:53:34Z

+  "dependencies": {
+    "@types/styled-components": "^5.1.25",


타입 관련 의존성은 개발환경에서 필요하기 떄문에 devDependencies 에 선언해주어도 좋아

아하 오키오키

joohaem · 2022-06-26T10:58:14Z

+  return data.documents;
+};
+
+export const getStoreBasedTown = async (town: string): Promise<Store[]> => {
+  const { data } = await api.get("", {
+    params: {
+      query: `${town} 노래방`,
+    },
+  });
+  return data.documents;
+};


getLocationBasedSearch 와 geStoreBasedTown 함수가 params 를 제외하고 비슷해보이는데
getQueryParams어쩌구 로 params 를 선언해주고 통합시켜주는 방법도 있을 것 같네!

그렇게 묶어봐야겠다 고마워!

joohaem · 2022-06-26T10:59:23Z

+    </StWrapper>
+  );
+}
+const StWrapper = styled.div`


시멘틱 태그도 함 고려해보심이 어떨까요,!

Suggested change

const StWrapper = styled.div`

const StWrapper = styled.main`

앗! 감사합니다 눈썰미좋네요

joohaem · 2022-06-26T11:01:00Z

+  }
+
+  & > .info__address {
+    max-width: 150px;
+  }


오호 클래스명으로 선언해주신 이유가 궁금합니다

노래방 카드 안의 정보 중에서 제목 빼고 나머지를 다 p태그로 해놔서 class로 구분했는데 nth-child로 바꾸는게 더 좋을까요?그때는 nth-child가 생각이 안났읍니다 ㅋㅋㅋ

+) 그냥 스컴포넌트로 만드는게 낫겠다고 생각이 들었습니다

SC 만들기 오케이 ~~~~

joohaem · 2022-06-26T11:01:56Z

+
+  return (
+    <StCard>
+      <a href={place_url || undefined}>{place_name}</a>


undefined 옵셔널을 붙여준 이유가 궁금합니다 !!

Suggested change

<a href={place_url || undefined}>{place_name}</a>

<a href={place_url}>{place_name}</a>

API 에 place_url 이 없는 것 떄문이라면 아래처럼 작성하는 게 더 명시적이지 않을까 합니다! (css 코드가 조금 변경될 수 있겠네요,!)

Suggested change

<a href={place_url || undefined}>{place_name}</a>

{place_url && <a href={place_url}>{place_name}</a>}

확인해보니까 불필요한 부분이네요... 꼼꼼하지 못했네요
만약 해당 데이터를 줄 수도 있고 안 줄수도 있는 경우라면 좜님이 밑에 써주신 코드로 작성하도록 할게요!👍

joohaem · 2022-06-26T11:06:49Z

+        <label htmlFor="text">우리 동네는 여기에요</label>
+        <input
+          type="text"
+          placeholder="지역을 입력해주세요."


여긴 id 속성이 빠진 것 같네요??

Suggested change

<label htmlFor="text">우리 동네는 여기에요</label>

<input

type="text"

placeholder="지역을 입력해주세요."

<label htmlFor="text">우리 동네는 여기에요</label>

<input

type="text"

id="text"

placeholder="지역을 입력해주세요."

시력 3.0이세요??

joohaem · 2022-06-26T11:09:31Z

+export default function ResultSection({
+  storeInfo,
+  isCheck,
+  isLoading,
+}: ResultSectionProps) {
+  return (
+    <>


컨벤션 차이이지만, 아래처럼 선언하기도 합니다! 가독성이 좋다는 의견이 있던 것 같아요,, 는 주함

Suggested change

export default function ResultSection({

storeInfo,

isCheck,

isLoading,

}: ResultSectionProps) {

return (

<>

export default function ResultSection(props: ResultSectionProps) {

const {storeInfo, isCheck, isLoading} = props

return (

<>

오..훨씬 보기 편하네요 감사합니다!

joohaem

@Nahee-Park

다른 언어들과는 달리 타입스크립트는 타입 추론이 되도록 유도하고, 타입이 추론되는 경우 굳이 타입을 명시해주지 않는 것이 컨벤션인 것 같습니다. 그런데 저도 이 부분에 대해 명확하게 왜 그렇게 하는 지 이유를 잘 모르겠습니다.

저도 궁금해서 여기저기 찾아봤는데,
다른 언어들같은 경우에는 메모리 영역을 위해 타입 지정이 필수적이므로 명시적으로 지정해주는 것 같고
자바스크립트와 타입스크립트는 메모리 할당 방식이 다르므로 생긴 컨벤션 차이가 아닐까 합니다!

명시적으로나 암시적으로나 타입 부여해주는 방식 둘 다 장단점이 있으니 목적에 따라 유연하게 쓰는 게 맞는 것 같아요
이를 위해 명시적 타입 지정을 위한 린트와 암시적 타입 지정을 위한 린트들이 도구로써 있는 것 같기도 하네요!!

참고 아티클 (링크)

leeseooo added 14 commits June 23, 2022 14:32

init: 초기 세팅

b3347a6

chore: 폴더 구조 세팅

38b80a5

chore: 스타일 및 타입 폴더 생성, 전역 스타일 추가

24b583c

feat: 검색 섹션 구현

f965535

feat: 지역기반 검색 api 연결

19a0b9f

feat: 검색결과 페이지 구현

b7cce01

chore: 지역기반검색 응답 data.documents 오타수정

795497d

feat: 스켈레톤 UI 추가

61fe632

feat: 동네 기반 위치검색 api 추가

a906ab3

feat: 카드 컴포넌트분리 및 구현

8e6d000

chore: 체크박스 체크 시 검색창 초기화

b28ed64

chore: 검색결과 없음 처리 추가

39bb6b0

chore: 지역기반일 때 거리 표출

c910b98

chore: 결과 출력 삼항연산자로 수정

332b879

leeseooo added the 4️⃣ 4주차 4주차 과제에요. label Jun 24, 2022

leeseooo requested review from Nahee-Park, joohaem and yunsun99 June 24, 2022 15:30

leeseooo self-assigned this Jun 24, 2022

joohaem approved these changes Jun 25, 2022

View reviewed changes

Nahee-Park reviewed Jun 26, 2022

View reviewed changes

joohaem approved these changes Jun 26, 2022

View reviewed changes

joohaem reviewed Jun 27, 2022

View reviewed changes

		const [isCheck, setIsCheck] = useState<boolean>(false);
		const [myTown, setMyTown] = useState<string>("");

	const onClickSearchButton = (isCheck: boolean, myTown: string) => {
	const onClickSearchButton = (isCheck: boolean, myTown: string, e:React.FormEvent<HTMLFormElement>) => {
	e.preventDefault();

	<a href={place_url \|\| undefined}>{place_name}</a>
	<a href={place_url}>{place_name}</a>

	<a href={place_url \|\| undefined}>{place_name}</a>
	{place_url && <a href={place_url}>{place_name}</a>}

Uh oh!

Conversation

leeseooo commented Jun 24, 2022

✨ 구현 기능 명세

🎁 PR Point

질문있어요

😭 어려웠던 점

😎 구현 결과물

Uh oh!

github-actions Bot commented Jun 24, 2022

Uh oh!

joohaem left a comment

Choose a reason for hiding this comment

Uh oh!

Nahee-Park left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joohaem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeseooo Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeseooo Jun 27, 2022 •

edited

Loading