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

Jihykim2 TODO, Web(2)(3) 제출 #15

Open
wants to merge 9 commits into
base: jihykim2
Choose a base branch
from
Open

Jihykim2 TODO, Web(2)(3) 제출 #15

wants to merge 9 commits into from

Conversation

jihyunk03
Copy link

@jihyunk03 jihyunk03 commented Aug 17, 2024

done

  • Todo list 마스터하기
  • Cabi 웹 페이지 만들기 (2) >> 보너스에서 모바일 관련 부분 미비
  • Cabi 웹 페이지 만들기 (3) >> 반응형(모바일 탭바) 미비

@jihyunk03 jihyunk03 self-assigned this Aug 17, 2024
@jihyunk03 jihyunk03 changed the title Jihykim2 TODO list 제출 Jihykim2 TODO, Web(2) 제출 Aug 18, 2024
@junyoung2015 junyoung2015 requested review from jnkeniaem and removed request for junyoung2015 September 10, 2024 05:21
html,
body,
div,
applet,
Copy link
Member

Choose a reason for hiding this comment

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

applet, h1같이 안 사용하는 태그 삭제하는게 깔끔해보일것같습니당

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 추가적으로 작성한 거면 몰라도, reset.css 에 기본적으로 포함된 요소들이라 극한의 최적화를 위해서가 아니라면 굳이 안 지워도 될 것 같다고 생각합니다...!
물론 지우면 그만큼 최종 크기는 줄겠지만요

Comment on lines +46 to +49
<li class="tab-bar__list__item">Home</li>
<li class="tab-bar__list__item item-selected">2층</li>
<li class="tab-bar__list__item">4층</li>
<li class="tab-bar__list__item">5층</li>
Copy link
Member

Choose a reason for hiding this comment

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

25~28행의 데이터와 중복되는데, 이렇게 두 군데 이상에서 쓰이는 값은 변수로 정의해 사용하는게 유지보수면에서 좋습니당

@jnkeniaem
Copy link
Member

html, css, js로 구현한 todo list와 cabi 섹션 페이지 잘 확인했습니다!
이제 리액트만 남았네요 리액트까지 화이팅입니다 💪🏻

@jihyunk03 jihyunk03 changed the title Jihykim2 TODO, Web(2) 제출 Jihykim2 TODO, Web(2)(3) 제출 Oct 10, 2024
Copy link
Member

@jnkeniaem jnkeniaem left a comment

Choose a reason for hiding this comment

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

굳굳! 거의 axios를 사용했지 fetch를 사용한 적이 드물고, 또 fetch는 공부할때 가볍게 익히고 넘어갔었는데, 이번 pr 리뷰 하면서 fetch를 보다 제대로 공부하게돼서 좋았습니당
코멘트 확인해주시고 온보딩하느라 고생많으셨습니다!

.then((data) => callback(null, data))
.catch((error) => callback(error, null));
}
callback(new Error("error in network"), null);
Copy link
Member

Choose a reason for hiding this comment

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

네트워크 에러는 then으로 가지 않고, fetch().then().catch() 에서 catch가 실행되는 걸로 알고있어요!

Copy link
Member

Choose a reason for hiding this comment

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

반면에 404, 500과 같은 서버 응답 오류는 then으로 가되, response.ok가 false 입니다.
따라서 이 부분은 네트워크 에러가 아니라 서버 응답 오류로 수정하는게 좋을것같습니다!

Copy link
Member

Choose a reason for hiding this comment

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

굳굳 👍🏻

if (response.ok) {
return response.json();
}
throw new Error("error in network");
Copy link
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.

3 participants