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

[ 4주차 기본/심화 과제 ] 🌞수수의 기상예보⛱️ #10

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

seojisoosoo
Copy link
Contributor

@seojisoosoo seojisoosoo commented May 12, 2023

✨ 구현 기능 명세

기본 과제

✅ 기상 카드

  1. 날씨에 따른 이미지
  2. 온도
  3. 체감온도
  4. 최저,최고
  5. 구름 %

✅ 일간 / 주간 + 지역(영어)검색

  1. 날씨 검색 타입
    1. 오늘 → 하루
    2. 주간 → 5일 예보
  2. 검색 기능
    1. /day/:area or /week/:area 와 같이 params로 검색한 지역을 넘겨줍니다. :: hint) useParams
    2. 이를 가져와 오늘/ 주간 날씨를 렌더링

✅ Outlet + Route

  1. Outlet과 Route를 활용하여 페이지 내의 컴포넌트를 변경해주세요!
    1. 헤더는 고정
    2. 기상 정보 카드들만 경로에 따라 변경됩니다.
  2. 에러페이지 처리도 필수!

심화 과제

✅ 스켈레톤 UI

  • 기상 정보를 불러올때, 사용자에게 스켈레톤 UI를 보이게 합니다!

✅ 커스텀훅으로 데이터를 받아옵시다!

  • 저 같은 경우에는 isError, isLoading, data를 커스텀훅의 반환값으로 만들어서 이를 스켈레톤 UI랑 연결지었습니다!!!

🌼 PR Point

common/weatherCard.jsx

setWeatherImg(WEATHER_TYPE.filter((w) => w.description === weatherData?.weather[0]?.description)[0]?.imgURL);
필터링을 통해서 description이 동일한 imgURL을 가져왔어요.

hooks/useWeather.jsx

return { fetchTodayWeatherInfo, fetchWeekWeatherInfo, isError, isLoading, todayData, weekDatas };
커스텀 훅을 이용해서 데이터 패칭 함수들과 데이터, 에러여부와 로딩여부를 관리했어요.

todayWeather, weekWeather

const { area } = useParams();
useParams를 이용해서 도시의 값을 읽어들였어요.


🥺 소요 시간, 어려웠던 점

  • 6h
  • 데이터 undefined문제! 옵셔널체이닝과 &&연산자로 거르면서 작업했어요!

🌈 구현 결과물

2023-05-12.4.38.46.mov

Copy link

@lydiacho lydiacho left a comment

Choose a reason for hiding this comment

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

언니꺼 코드리뷰하면서 느낀점

  1. 함수 분리를 너무너무 잘해줘서 코드가 너무 직관적이고 술술 읽혀서 코드리뷰하는데 기분이 좋다
  2. 심화과제까지 구현하면서 딱 필요한 알짜배기만 6시간만에 부셔버리는 폼 진짜 미쳤다 솝커톤을 앞둔 나에게 많은 감명을 준당 . 언니처럼 되고싶드앙

);

return data.data;
}

Choose a reason for hiding this comment

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

와 내가 딱 이렇게 API만 분리한 파일 만들어보고 싶었는데 이렇게 하면 앞으로 API 폴더 빼서 이렇게 하면 되겠다!!!! 기능 분리 최고

@@ -0,0 +1,33 @@
import React, { useState } from "react";
import { getTodayWeather, getWeekWeather } from "../api/getWeather";

Choose a reason for hiding this comment

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

단순히 훅에서 API 통신 구현하는게 아니라 API 통신 스크립트 따로 빼두고 함수명으로 비동기 호출하니까 훨씬 직관적인 것 같아 너무 깔끔하다!!! 코드리뷰 하는데도 이해하기 넘 좋아

Comment on lines +11 to +18
try {
setIsLoading(true);
const response = await getTodayWeather(area);
setIsLoading(false);
setTodayData(response);
} catch (e) {
setIsError(true);
}

Choose a reason for hiding this comment

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

트라이 캐치... 어싱크 어웨이트.. 킹갓지수 언니가 얘기해준거 명심하겠슴니다

}
}

return { fetchTodayWeatherInfo, fetchWeekWeatherInfo, isError, isLoading, todayData, weekDatas };

Choose a reason for hiding this comment

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

함수 넘겨주는건 생각도 못했는데!! 함수는 무조건 export import 형태만 생각했었거등 이렇게 넘겨주는게 더 훅스러운건가???_? useState 함수 props로 넘겨주는 것도 언니한테 배워서 유용히 썼었는데 결국 얘도 훅스니까 이렇게 넘겨주면 되겟군아 넘 좋아

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 경우에는 함수안에서 변경된 변수들(isLoading, weekDatas, isError)도 다 사용이 되어야하니까, 함수 자체로 빼기 보다는 커스텀 훅을 사용하는 게 더 좋다고 판단했어용!! 경우에 따라 유용하게 사용하면 좋을 것 같오요~

`;

const Title = styled.h1`
${({ theme }) => theme.fonts.header};

Choose a reason for hiding this comment

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

font도 theme으로 적용! 깰꼬매요

return (
<WeatherCardWrapper>
<h1>{title}</h1>
<img src={weatherImg} alt={title} />

Choose a reason for hiding this comment

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

리액트하면서 자꾸 alt 까먹게 되더라 잊지않고 꼬박꼬박 쓰는거 조하욥

Comment on lines +7 to +9
<Title></Title>
<Img></Img>
<Text></Text>

Choose a reason for hiding this comment

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

질문! 요런건 그냥 어차피 InnerHTML 안들어가니까

<Title/>

이렇게 들어가면 안되는걸까유??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 완전 맞아요!! 그렇게 고쳐야징

Comment on lines +20 to +25
if (isError) {
return <ErrorPage />;
}

return <>{isLoading ? <SkeletonTemplate /> : <WeatherCard weatherData={todayData} title={todayData.name} />}</>;
}

Choose a reason for hiding this comment

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

오오오 여기 리턴으로 isError 조건까지 한번에 처리해줄 수 잇지 아늘까?!

Suggested change
if (isError) {
return <ErrorPage />;
}
return <>{isLoading ? <SkeletonTemplate /> : <WeatherCard weatherData={todayData} title={todayData.name} />}</>;
}
return <>{isError? <ErrorPage /> :
isLoading ? <SkeletonTemplate /> : <WeatherCard weatherData={todayData} title={todayData.name} />}</>;
}

요런 늑낌스로.. 소심한 제안

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 방법도 죠타! 보통 공식문서들에서

  if (isError) {
    return <ErrorPage />;
  }

이렇게 많이들 쓰길래 이렇게 표현해보았는데, 숭희 제안대로 써보는 것도 좋은 것 같아용!!

Comment on lines +32 to +35
<Input type="text" placeholder="영어로 도시명 ex) Seoul" onChange={getCity} />
<Button type="button" onClick={searchWeather}>
날씨 검색
</Button>

Choose a reason for hiding this comment

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

질문!! 여기 부분 Search 컴포넌트로 따로 빼둔것 같은데 그 컴포넌트 안쓴 이유가 있을까유?? 내가 그 컴포넌트의 쓰임을 못찾은건가!??!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헤헤 따로 뺐다가 의존성이 높은 것 같아서 다시 합쵸보림! 눈썰미 짱이다..!!

Comment on lines +67 to +70
&:focus {
outline: none;
border: none;
}

Choose a reason for hiding this comment

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

focus css 준거 야무져유

Copy link
Member

@Chanwoo-Jeong Chanwoo-Jeong left a comment

Choose a reason for hiding this comment

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

역시 서잔리드수 최고 짱짱 정말 깔끔해요!

<p>
최저/최고 | {main?.temp_min}/{main?.temp_max}
</p>
<p>구름 | {clouds?.all}</p>
Copy link
Member

Choose a reason for hiding this comment

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

| 이거 이런식으로 하는구나!
나는 보통 after: content 처리해서 가상선택자로 만들어주거든!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오오 이거 좋다!! 오빠는 before after 활용을 참 잘하는 것 같아용!


if (isError) {
return <ErrorPage />;
}
Copy link
Member

Choose a reason for hiding this comment

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

뭔가 이렇게 미리 돌리는게 좋은가?!
뭔가 가독성 적으로는 early return 같은 느낌이라 좋은거같네!
나도 한번 이렇게 짜봐야겟다 나는 맨날

isError ? : isLoading ? : 약간 이런식으로 했거든!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오호! 둘 다 좋은 방법인 것 같긴 해용! 공식 문서들에서 보통 저렇게 미리 돌리길래 따라해봐쎠!

//오늘 날씨 조회
export async function getTodayWeather(area) {
const data = await axios.get(
`https://api.openweathermap.org/data/2.5/weather?q=${area}&appid=${import.meta.env.VITE_APP_WEATHER}&units=metric`,
Copy link
Member

Choose a reason for hiding this comment

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

나도 다 하고 깨달았는데 weather 과 forecast 만 변수처리해주면 이 api 도 하나로 묶을 수 있을거같아!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants