Skip to content

날씨앱 과제 최종 완성#17

Open
KimJIHoon124 wants to merge 2 commits intoTEAM-ALOM:mainfrom
KimJIHoon124:main
Open

날씨앱 과제 최종 완성#17
KimJIHoon124 wants to merge 2 commits intoTEAM-ALOM:mainfrom
KimJIHoon124:main

Conversation

@KimJIHoon124
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

@seunghui-park seunghui-park left a comment

Choose a reason for hiding this comment

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

과제하느라 고생 많으셨습니다!
8주차 팀플 파이팅 하십셔 !!!! 3팀 팟팅 ~~

const CurrentWeather = ({ weatherData, isLoading }) => {
if (isLoading) {
return <div>채워주세요</div>;
return <div>현재날씨 로딩중...</div>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

메시지 하드 코딩 보다는 별도의 컴포넌트 활용해 보시면 좋을 듯 합니당

Comment on lines +42 to +46
<h3>
현재 위치 : 서울
<br />
현재 시각 : {new Date(currentData.time).getHours()}시
</h3>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

h3 태그 안에 br 태그를 사용하는 건 시멘틱 구조 상 좋지 않아 보입니다!
별도의 태그로 감싸는 게 더 좋을 것 같아요

Comment thread src/utils/weather.js

const hourlyData = [];

for (let i = 0; i < 12; i++) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

여기서는 크게 상관 없을 수 있지만,
여러 곳에서 사용 되거나 로직 상 의미가 있거나 자주 변경될 가능성이 있는 값은 상수 분리를 하는 게 좋습니다! 다음부터는 상수 분리에 대한 고민도 하면서 구현하시면 좋을 것 같습니당

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants