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

KDT0_KimMinSeob 사진관리서비스 #63

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

Conversation

LikeFireAndSky
Copy link

@LikeFireAndSky LikeFireAndSky commented Aug 18, 2023

✨ 계곡 포인트 사진 관리 서비스


✅ 개요

  • 계곡에서 놀 수 있는 포인트를 관리하는 서비스를 만든다.
  • Firebase Storage를 통해서 cors를 해본다.
  • 반응형 웹과 애니메이션을 구현해본다.

✅ 기능

  • 메인 페이지 기능
  • 사진 추가 기능
  • 정보 추가 기능
  • 위로 가는 버튼 기능
  • 사진, 정보 수정, 삭제 기능
  • 연관 추천 기능

✅ 사이트 주소

사이트주소

✅ 유저 플로우

사진

✅ 배운점

  • 웹펙사용법
  • 컴포넌트형 웹 구성 방법
  • IntersectionObserverApi 사용법
  • 데이터 전송, 받아오기 하는 방법
  • 피드백 받은 내용 최대한 반영해서 문제되지 않도록 주의

✅ 고쳐야할 점

  • 마지막에 시간 부족해서 반응형 observerApi 구현 불가
  • 중간 중간 css 요소 이상한 부분들 수정 필요
  • JS 클린 코드 매우 필요
  • 상태 중복 문제
  • lazyloading 미구현

@SOL-MI
Copy link

SOL-MI commented Aug 18, 2023

헉 사이트가 멋져서 구경하다가 기차계곡의 사진이 사라져버렸습니다..! 수정하기 버튼을 눌러봤는데 취소가 없어서 아무것도 안 건드리고 수정하기-확인을 눌렀는데 사진이 사라지네요ㅠㅠ 사이트 너무 멋있습니다:D 확실한 컨셉과 지도를 활용하신 것이 인상깊었습니다..!

@LikeFireAndSky
Copy link
Author

헉 사이트가 멋져서 구경하다가 기차계곡의 사진이 사라져버렸습니다..! 수정하기 버튼을 눌러봤는데 취소가 없어서 아무것도 안 건드리고 수정하기-확인을 눌렀는데 사진이 사라지네요ㅠㅠ 사이트 너무 멋있습니다:D 확실한 컨셉과 지도를 활용하신 것이 인상깊었습니다..!

좋게 봐주셨다니 정말 감사드립니다...!! 해당 이슈는 수정 페이지에 들어가면 수정 또는 삭제를 무조건 해야 하는 것 때문에 발생하는 것 같습니다...🥲 리펙토링을 통해서 고쳐나가고 있습니다...!! 부족하지만 이렇게 직접 방문해주셔서 코멘트까지 남겨주셔서 너무 감사드립니다!! 늦게 확인해서 정말 죄송합니다. 솔미님 덕분에 한 가지 더 배워가는 것 같습니다 정말 감사합니다 😊

Comment on lines 30 to 34
const setData = async () => {
const date = new Date();
const dateId = Store.state.nickname + date.getTime().toString();
const dateId = 'valleyId' + date.getTime().toString();

await uploadImage(dateId);
Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

사진

ID값이 한글일 때 about 페이지에서 보이지 않는 문제가 발생

변경 내용

ID 값을 nickname이 아닌 고정 sting 값으로 변경

해결 과정

ID 값이 한글 일 때 list페이지에서는 보여지지만 about페이지에서 보여지지 않는 이유를 생각해봤을 때 about 페이지에서 데이터를 보기 위해서 ID를 query를 통해서 쐈기 때문에 한글이 query로 들어가서 문제가 발생하는 것이라고 생각해서 ID에 한글이 들어가지 않게 독립적인 ID로 변경하였음

리뷰관련

ID값이 현재 유닉스 데이트타임이기 때문에 보안상으로 취약할 수 있다는 문제가 야기 될 것 같다고 생각합니다. 따라서 ID값을 자동으로 지정해주는 기능을 쓰거나 암호화를 통해서 해당 문제를 해결해 볼 수 있을 것 같습니다.

Comment on lines 41 to 54
.modal__wrapper {
width: 90%;
height: 70%;

overflow: hidden;
}
}

@media (max-width: 400px) {
.modal__wrapper {
width: 90%;
height: 55%;

overflow: hidden;
Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

사진

반응형 미디어 쿼리에서 모바일로 넘어가는 부분에서 등록 모달 창이 스크롤이 되지 않는 이슈 발생

변경 내용

overflow hidden 삭제

해결 과정

피어 리뷰를 진행하던 중 팀원 분들이 말씀해 주셔서 스크롤이 안돼서 넘어가 지지 않는 오류를 찾았고 이를 고침

리뷰 관련

반응형 관련해서 주의를 기울여야 할 것 같습니다.

@moonyah
Copy link

moonyah commented Aug 25, 2023

과제하느라 넘넘 수고하셨어요~~ 제가 추가한 정보에 대한 상세 페이지가 안 떠서 삭제를 못했어용 ㅜㅜ 추가로 뒤로 가기나 닫기 버튼 있으면 너무 좋을 것 같습니다! 그리고 검색 기능 구현 너무 잘된 것 같습니다~~! 저도 기능 추가해봐야겠네요.

@seungjun222
Copy link

seungjun222 commented Aug 25, 2023

민섭님 src/core/index.js에서 component, router, createStore 로직 깔끔하게 잘 구현하셨네요!! dispatch/index.js도 잘 봤습니다! components 폴더에서 클래스형 컴포넌트 이해도도 되게 좋으시고, 진짜 코드 정성이 장난 아니네요..
많이 반성하고 배우게 됩니다!!

@lilviolie
Copy link

프로젝트 고생하셨습니다!! 일관성있는 주제선정과 열정 본받고 갑니다..! 지도 주소와 연관 계곡도 아이디어 엄청 좋은것같아요ㅎㅎ 근데 계곡 등록 후 해당 계곡의 상세페이지로 잘 이동이 되지 않거나 삭제가 안되네요😭

@LikeFireAndSky
Copy link
Author

과제하느라 넘넘 수고하셨어요~~ 제가 추가한 정보에 대한 상세 페이지가 안 떠서 삭제를 못했어용 ㅜㅜ 추가로 뒤로 가기나 닫기 버튼 있으면 너무 좋을 것 같습니다! 그리고 검색 기능 구현 너무 잘된 것 같습니다~~! 저도 기능 추가해봐야겠네요.

양자택일의 기능이었습니다... 뒤로 가기 버튼 빠르게 리펙토링을 통해 구현했고, 월요일날 리펙토링 마저 끝내고 다시 배포 사이트에 업데이트 하겠습니다!!

제가 구현해 본 검색 기능은 문제가 데이터의 양이 많아지면 정말 메모리를 엄청나게 많이 잡아먹는 기능입니다 ㅠㅜㅜ 모든 배열을 문자열로 바꿔서 검색하는 것이라 이번 CS 스터디에서 배웠던 시간복잡도의 측면에서 봤을 때 아마 O(n^2)의 시간복잡도를 가지는 것 같습니다. 그 이상일 것 같기도 해요.... firebase에서 검색엔진이 따로 마련되지 않아서 외부 paas 프로그램을 사용해야 하더라고요!!

UI가 요즘 중요시되면서 FE선에서 더 많은 기능을 구현해야 한다는 사실을 컬럼을 읽으면서 알게 되었습니다. 따라서 API를 요청하는 것 이외에도 FE에서 시도해볼 수 있는 기능들을 조금 더 알아봐야겠다고 생각이 들었습니다. 더 고민해보고 좋은 방법을 알게 된다면 공유하겠습니다 🥲 또 좋은 방법이 있다면 공유해주세요 👻

@LikeFireAndSky
Copy link
Author

LikeFireAndSky commented Aug 25, 2023

프로젝트 고생하셨습니다!! 일관성있는 주제선정과 열정 본받고 갑니다..! 지도 주소와 연관 계곡도 아이디어 엄청 좋은것같아요ㅎㅎ 근데 계곡 등록 후 해당 계곡의 상세페이지로 잘 이동이 되지 않거나 삭제가 안되네요😭

상세페이지로 이동하지 않는 이유를 봤을 때 query를 통해 이동할 때 문제가 발생하더라고요

문제 발생 부분

처음에 각 데이터의 ID에다가 수업에서 배웠던 유닉스 타임을 더해서 사용했습니다. 그것이 문제가 되어 쿼리를 쏠 때 문제가 발생했었습니다.

이에 ID를 사용자가 등록한 nickname과 별개로 작동되게 하였습니다.

그리고 은지님 덕분에 lodash 라이브러리 알게 되어서 정말 유용하게 사용했습니다!! 연관 콘텐츠 보여주는 부분에서 중복 제거할 때 정말 유용하더라고요!! 감사합니다 😊

@LikeFireAndSky
Copy link
Author

LikeFireAndSky commented Aug 25, 2023

민섭님 src/core/index.js에서 component, router, createStore 로직 깔끔하게 잘 구현하셨네요!! dispatch/index.js도 잘 봤습니다! components 폴더에서 클래스형 컴포넌트 이해도도 되게 좋으시고, 진짜 코드 정성이 장난 아니네요.. 많이 반성하고 배우게 됩니다!!

src/core/index.js에서 component, router, createStor 이 로직은 필수 강의에서 알려준 내용이었습니다!! 클라이언트 측에서 전역 상태를 유지하고 필요한 데이터만을 리렌더링 없이 구현해보자는 생각으로 다음 주차 필수 강의를 먼저 듣고 이를 활용해서 이번 과제를 진행해 보았습니다!!

dispatch/index 부분은 처음에는 db를 모든 곳에서 선언했고 메소드로 함수를 사용해야 하는 경우도 발생해서 아예 하나의 파일에서 모든 API 요청을 처리하는 것이 더 이상적이라고 판단해서 한번 구성해봤습니다!!

이번 Component형 웹을 만들어보면서 느꼈던 것들은 클라이언트 측에서의 상태를 관리하는 것은 편한데 서버에서 계속해서 데이터를 받아서 저장해야 하니까 로직이 복잡해지고, 페이지를 왔다 갔다 하면 상태가 중첩되서 API 요청이 배수로 증가하는 문제가 있었습니다...

그래서 저번에 알려주셨던 상태관리 툴에 대해서 배워봤고 서버의 데이터 생태 주기와 클라이언트의 생태 주기를 나누는 것이 중요하다는 사실도 알게 되었습니다. 그래서 react-query나 swr 같은 서버 수준에서 상태를 관리하는 라이브러리들을 사용하는 것이더라고요!!

이번 과제에서도 저번에 알려주셨던 상태관리 툴과 CSS 방법론이 정말 큰 도움이 됐고 항상 좋은 인사이트를 알려주셔서 덕분에 많은 것들을 배우고 있습니다. 정말 매번 감사합니다 승준님!! 👻

Copy link

@minsoo-web minsoo-web left a comment

Choose a reason for hiding this comment

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

민섭님 고생 많으셨습니다!

필수 강의에서 영감을 얻으신 방법으로 구현하셨군요!! 재밌으셨는지 html과 pure한 script 로 DOM을 조작하는 것과 어떤 차이가 있었는지 궁금합니다.

전반적으로 관심사의 분리와 변수명 개선이 필요해보였습니다.

고생 많으셨습니다!

"version": "1.0.0",
"description": "직원들의 사진을 관리할 수 있는 사진 관리자 서비스를 만들어 보세요.",
"main": "index.js",
"dependencies": {

Choose a reason for hiding this comment

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

사용되지 않는 의존성 파일들이 많이 보이는 것 같은데, 이유가 있을까요?

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

이번 프로젝트는 정말 재밌게 만들었던 것 같습니다 😎
먼저 class를 왜 쓰는 지 조금 알 수 있었습니다. constructor와 extends를 통한 상속이 정말 강력한 기능을 가지고 있다는 것을 배울 수 있었습니다. 또한 컴포넌트를 재활용해서 사진을 보여주는 기능에 사용해서 시간을 절약할 수 있었습니다.
두 번째로 저번 피드백을 수용해서 css 디자인을 하니 css를 활용하는데 있어서 어려움이 없었어서 디자인을 수월하게 했던 것 같습니다. 무조건 container-wrapper-item이라는 규칙을 정하고 나니 디자인할 때 어디에 relative를 주어야 할 지, 어디에 absolute를 줘야 할 지 명확하게 판단할 수 있었고 vw를 활용한 반응형 제작 경험도 DX측면에서 상당히 좋았던 것 같습니다. 기본이 rem인 tailwindcss같은 라이브러리들을 빨리 써보고 싶은 마음도 생겼습니다..!!
세 번째로 주말에 읽었던 react와 맥락이 비슷한 컴포넌트형 웹을 만들어보면서 어떠한 식으로 react가 진행되는 지 알 수 있게 되었습니다. 특히 core/index를 강의를 통해 제작해보고 컴포넌트들을 실제로 커스텀하여 사용하면서 왜 리엑트가 프레임워크가 아닌 라이브러리인 지 알 수 있게 되었습니다.
마지막으로 Store의 중요성에 대해서 알 수 있었습니다. 컴포넌트형 웹을 제작해보면서 전역 상태관리가 왜 필요한 지 알 수 있었습니다. 모달창이 열렸는 지, 그리고 닫혔는 지 판단하는 것부터 저장된 데이터를 다시 불러오지 않고 스토어에 저장된 데이터를 활용할 수 있다는 것을 상태관리를 통해서 알 수 있게 되었습니다. 다만 생태 주기가 있는 데이터를 관리하는 것에 대해서 이번 프로젝트를 통해서는 아직 이해를 하지 못하였던 것 같습니다. 상태 주기가 모두 load 기준으로 설정된 것 같아 이러한 부분을 더 공부해야겠다고 다짐했습니다.

다음 맥락으로 html과 pure한 script로 DOM을 조작하는 것과 어떤 차이점이 있었는 지 생각해보면, 검색과 연관 콘텐츠 추천을 이번에 웹을 구성할 때 필수로 만들어보자고 생각하고 제작하였는데, 만약에 해당 기능을 pure한 script와 DOM을 조작해서 만들었다면 페이지를 즉시 업데이트 할 수 없고 컴포넌트를 재 사용할 수 없기 때문에 일일히 다 작성해줘야 하는 단점이 있었을 것 같습니다. 또한 부분적으로 DOM을 렌더링을 할 수 없기 때문에 특히 웹의 구조를 유지하기가 어려울 것 같습니다. 그리고 길고 긴 HTML 코드를 보는 것이 아니라 모듈형식의 파일을 보니 코드를 만들기가 더 수월했던 것 같습니다!!

발생 오류

사용하지 않는 의존성 파일들이 사용됨

변경 내용

변경한 내용
사용하지 않는 의존성 패키지들을 찾아서 json파일에서 삭제하고, 사용하고 있는 의존성 파일들에 대한 markdown 작성

해결 과정

사용하지 않는 의존성 파일들이 많아 보이는 이유에 대해서 알아보았을 때 기존에 개발 의존성 패키지로 설치하지 않은 파일들이 node_modules에 global하게 남아있어서 그런 것 같다고 생각합니다.

따라서 이를 해결하기 위해 방법을 찾아본 결과
npm library 중 depcheck를 이용해서 사용하지 않은 dependency들을 찾고 삭제하였습니다.

이후 dependency 관리를 위해 readme를 적어두는 것이 이후 수정과 유지 보수에 필요하다고 하여 marckdown을 추가하였습니다.

리뷰 관련

사용되지 않는 의존성 파일들을 추가하는 것이 웹 성능에 문제가 될 수 있다는 사실을 알 수 있게 되었습니다. 또한 npm library들을 다운 받을 때 전역적으로 필요한 것들만 다운 받고 나머지 부분들은 개발 의존성 패키지로 다운 받아야 한다는 것을 알 수 있었습니다.

src/App.js Outdated

Choose a reason for hiding this comment

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

4조의 가현님과 코드베이스가 굉장히 비슷하네요 !
가현님에게 남긴 리뷰를 같이 확인해보시는 게 좋을 것 같아요

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

질문 리뷰
가현님에게 남겨주신 리뷰를 모두 확인해봤습니다!!

남겨주신 해당 리뷰에서는

  - page.js // or component.js or index.js
  - styles.css
  - script.js 

와 같이 구성하시라고 말씀해주셨습니다.
만약 이렇게 구성을 한다면 폴더 하나(Edit이라는 하나의 관심사를 가진 폴더)에 index.js파일과 해당 파일에서 사용할 컴포넌트들, css파일들을 함께 넣어서 관리하는 것으로 구성을 하는 것이라고 저는 이해를 하였습니다. 또한 react 공식 문서를 읽고 있는데 해당 기능처럼 css를 기능별로 자동으로 구성해주는 것을 알 수 있었습니다. 그렇다면 이러한 방식이 저번 멘토링 때 말씀해주신 도메인 주도 관리 개발 방법론을 실제로 사용할 수 있는 방법인 지 궁금합니다!!

Choose a reason for hiding this comment

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

이 방식은 CDD에 조금 더 가깝구요, 도메인 주도 개발 방법은 더 큰 범위 입니다.

도메인을 중심으로 컴포넌트들을 관리할 것이냐,
아니면 기능에 초점을 맞춰서 컴포넌트들을 관리할 것이냐,
아니면 컴포넌트의 크기 및 의존성에 따라서 컴포넌트를 관리할 것이냐,

의 차이라고 봐주시면 되고, 컴포넌트를 관리하는 기법입니다.

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 30, 2023

Choose a reason for hiding this comment

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

감사합니다!! 컴포넌트형 개발 주도 방식(bottom-up)에 대해서 알 수 있었습니다. 팀플을 할 때 유용하게 사용할 수 있는 방법이라고 알 수 있었습니다. 컴포넌트 단위로 작업하다 보니까 작업이 겹칠 일이 없을 것 같습니다. 또한 이런 구성을 편하게하는 Storybook 같은 서비스도 있더라고요!! 저번에 공식 문서에서 한번 봤었는데 토이프로젝트 때 배워서 써봐야겠습니다

export default class AddMember extends Component {
constructor() {
super({
tagName: 'div',

Choose a reason for hiding this comment

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

tagName을 안 받으면 div로 되어있어서 불필요한 코드 같습니다

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

발생 오류

tagName이 중복 선언됨

변경 내용

변경한 내용
super({}) 부분 삭제
( super()를 다 지워버려서 이후 super()만 남겼습니다.)

해결 과정

부모 Component Class에 이미 div tag를 default값으로 설정해뒀는데 다시 선언해서 이를 수정함

리뷰 관련

불필요한 코드들을 사용하지 않을 것을 계속해서 명심해야겠습니다.

Comment on lines 11 to 17
const state = this.props;
let stateValue = '';
if (state === 'update') {
stateValue = '업데이트';
} else if (state === 'delete') {
stateValue = '삭제';
}

Choose a reason for hiding this comment

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

state가 update나, delete가 아닐 경우의 예외처리를 만들거나, 아니면 else if -> else 로 바꾸는 작업이 필요해보입니다!

Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

state의 상태에 대한 예외 처리 기능이 부재

변경 내용

변경한 내용
else를 추가하여 예외 처리를 만듦

해결 과정

state가 update나 delete가 아닌 경우는 무조건 적으로 실행이 되면 안되기 때문에 강제로 창을 종료 시키도록 코드를 추가하였습니다.

리뷰 관련

부모 요소에서 props를 가져오거나 아니면 fetch를 통해서 api를 통해 data를 가져오거나 할 때 예외 처리를 위해 모든 구문에 try catch를 하는 것이 궁금합니다. api를 통해 crud를 할 때는 물론 네트워크라는 문제가 발생할 수 있기 때문에 graceful하게 코드를 작성하려면 필요하다고 생각합니다!! (다른 팀원들 코드 리뷰를 진행하면서 알 수 있었습니다) 만약에 그렇다면 props를 가져오는 상황에서 try catch를 이용한 예외 처리도 필요한 지 궁금합니다!! props는 호출을 할 때 무조건 받아 올 수 있다고 생각했었습니다.

Copy link

@minsoo-web minsoo-web Aug 28, 2023

Choose a reason for hiding this comment

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

우선 props로 받아올 때 try/catch를 하는 것은 모든 함수에서 인자값이 제대로 받아졌는지를 검증하는 것과 마찬가지이기 때문에 불필요합니다.

하지만, api call 의 상황에서는 어떤 데이터가 올지, 성공의 여부를 보장할 수 없기 때문에 try/catch를 매번 사용하며 예외처리를 하는 것이 올바른 습관이라고 생각합니다.

이 리뷰에서는 state가 'update' 이거나, 'delete' 인 상황이니까
else if 일 필요가 없고,
ts를 사용해서 type을 강화하는 게 아니니까 사용자가 이상한 state를 props로 잘못 넘겨줄 수도 있자나요?
그런 경우를 방지하고자 예외처리를 하는 게 좋아보인다. 라는 말이었습니다.

예를 들어서,

const foo = (param) => {
// some
}

이런 함수가 있을때,
foo();

라고 호출하는 사람이 없자나요?

근데 param이 'a' 또는 'b' 인 경우만 가능한 foo함수라고 쳤을 때,
'c'를 넣어주면 잘못 사용했다고 알려주는 건 필요합니다. 이런 불필요한 과정을 겪지 않게 해주는 게 ts의 장점이라 생각합니다.

Copy link
Author

Choose a reason for hiding this comment

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

이번에 ts에 대해서 배우면서 필요성을 한번 더 상기 시킬 수 있었습니다. context에서가 아니라 component안에서 예외 처리를 할 수 있는 좋은 기능인 것 같습니다. 열심히 배우고 있습니다 😊 하지만 아직 DOM과 같은 특수한 타입들에 대해서 어떻게 다가가야 할 지 감이 잡히지 않는 것 같습니다. 공식 문서를 보면서 어느 정도 해결해보고 있기는 한데 특히 파이어베이스 같은 경우의 서드 파티 라이브러리를 사용할 때 타입을 지정해주는 방법론이 쉽지가 않은 것 같습니다 🥲

});

const button = this.el.querySelector('.confetti__button');
const end = Date.now() + 3 * 1000;

Choose a reason for hiding this comment

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

end가 어떤 변수일까요..?! 변수명을 조금 더 명확하게 지었다면 좋았을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

변수 명이 명확하지 않아서 변수 파악이 어려움

변경 내용

변경한 내용
변수 명을 end에서 confettiEndTime으로 변경하였고 그 외 다른 general한 변수들의 이름도 변경하였음

해결 과정

변수들의 이름이 너무 general하기 때문에 코드가 길어질 경우 해당 변수가 어떤 역할을 하는 지 파악하기가 힘들 수 있기 때문에 변수 명을 알아보기 편하게 변경하였음

리뷰 관련

변수 명을 주의해서 적도록 하겠습니다.

this.el.classList.remove('active');
});

const inputEls = this.el.querySelectorAll('input');

Choose a reason for hiding this comment

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

다른 분 리뷰에 남겼던 것 같은데,

el, btn, e, i 와 같이 불필요하게 많이 줄인 변수명은 오히려 가독성을 낮춥니다.
에디터의 도움으로 어차피 자동완성이 가능하니, 변수명은 최대한 길게 명확하게 적는 것을 추천드립니다!

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

발생 오류

el, btn과 같은 축약 변수 명의 가독성 저하 요소

변경 내용

변경한 내용
el, btn을 각 역할에 맞는 변수 명으로 변경하였습니다.
(예를 들어 inputEl => inputValuesElement로 변경하였습니다.)

해결 과정

모든 js파일에 축약어로 된 변수 명을 다시 역할에 맞게 변경하였습니다.

리뷰 관련

강의에서 사용했던 el과 btn등의 요소들이 가독성을 늘리는 방식이라고 생각했었는데 오히려 축약하니 어떤 기능을 하는 element이고 어떤 input을 받는 element인 지 오히려 더 모르게 되는 것 같습니다. 변수 명을 사용할 때는 역할에 맞게 구성하는 것이 가장 우선이라는 것을 배웠습니다!

Choose a reason for hiding this comment

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

민섭님두, 코드를 실행하는 부분과, 함수를 정의하는 부분, DOM 을 그리는 부분을 관심사에 맞게 나눠서 관리했으면 더 좋았을 것 같습니다.

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

발생 오류

class Component에 render안에 모든 기능이 들어가 있어서 코드를 보기 힘듦

변경 내용

변경한 내용
변경한 내용 추가 수정
class의 함수 만들어서 각각의 기능을 분리함.
함수 내부에서 실행되어야 하는 함수들을 따로 정의하고 this.함수 를 통해서 함수를 실행

해결 과정

render()에 집중되어있던 기능들을 redner() 함수 밖으로 빼서 다시 정의한 후 constructor에서 호출하여 사용하였습니다.

리뷰 관련

render 를 제외하고 다른 메소드들을 추가해주는 방향으로 리팩토링해보면 어떨까요?
리스너 같은 경우는 render 가 실행되고 난 다음에 실행되어야 하니, 그런 부분들도 신경써서 처리해보시면 재밌을 것 같아요!

라는 가현님의 코드 리뷰를 남겨주셨었던 것에서 질문을 드립니다!! Component core함수에서 render()을 자동으로 실행하도록 구성하였는데 이렇게 constructor에서 호출을 하면 render()가 우선 실행 되고 나서 호출한 함수들을 실행한다고 알 고 있습니다. 여기에 추가적인 처리를 해야 말씀해주신 기능이 실행이 되는 것인지, 아니면 코드 수정한 대로 constructor 에서 함수를 호출하여 render()가 실행되고 나서 호출한 함수가 실행되는 것인지 궁금합니다.

Choose a reason for hiding this comment

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

render가 실행되고 나서 호출되어야 할겁니다.
dom에 요소가 생기고 난 다음에, 이벤트리스너가 접근할 수 있으니까요!

Copy link
Author

Choose a reason for hiding this comment

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

관심사를 분리하는 기준을 일단 크게

  1. html을 그리는 부분
  2. eventListener가 작동하는 부분
  3. 메소드를 지정하는 부분

이렇게 큰 갈래로만 나눈 것 같습니다.
관심사 분리에 대해서 알아보니까
크게 View와 Logic으로 분리해서도 활용을 하는 것을 알게 됐습니다.

멘토님 덕분에 관심사 분리라는 개념에 대해서 알 수 있게 되었습니다. 좋은 인사이트를 주셔서 감사합니다!!

Copy link
Author

Choose a reason for hiding this comment

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

한 가지 궁금한 점은 이번에 리엑트 공식 문서를 읽으면서 state는 최소한으로 사용해서 쓰고 나머지는 props로 사용해야 한다고 배웠습니다.

관심사를 분리했다면 View보다는 Logic에 state상태를 두어야 한다는 것도 알 수 있었습니다.

그렇다면 평소에 state를 관리하실 때 어떻게 관리하시는 지 궁금합니다!! 이번 과제의 경우에서는 그냥 필요한 곳마다 State를 추가해서 썼는데 이를 개선해보고 싶습니다.
component instance에서 state를 사용하는 경우와 클라이언트 상태, 서버 상태에서 어떻게 그리고 언제 state를 사용할 지 판단하는 기준에 대해서 알고 싶습니다.

@@ -0,0 +1,46 @@
<!-- 아무것도아님 -->

Choose a reason for hiding this comment

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

아무것도 아닌 파일을 제출까지한 프로젝트에는 안 올렸으면 어땠을까요..!

Copy link
Author

@LikeFireAndSky LikeFireAndSky Aug 28, 2023

Choose a reason for hiding this comment

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

변경 내용

해당 파일을 삭제하였습니다.

리뷰 관련

innerHTML을 사용하면 말씀해주셨던 html 태그 익스텐션이 작동을 안 하여 innerHTML 작성 용으로 만들었던 파일을 그대로 push하였습니다. 마지막 처음 방문 페이지를 추가해보고자 시간 분배를 못하여 급하게 올리느라 체크를 하지 못했습니다. 앞으로 시간을 주의해서 사용하며 코드를 체크하고 push하도록 하겠습니다!

Comment on lines 10 to 19
const firebaseConfig = {
apiKey: 'AIzaSyDiHT2Ps0faE1rGWSDVj5wI9gUjzYubCdQ',
authDomain: 'fcjsspa.firebaseapp.com',
databaseURL: 'https://fcjsspa-default-rtdb.asia-southeast1.firebasedatabase.app',
projectId: 'fcjsspa',
storageBucket: 'fcjsspa.appspot.com',
messagingSenderId: '381168199169',
appId: '1:381168199169:web:3d03879e113bcfa3750200',
measurementId: 'G-CTPG9J3C0Q',
};

Choose a reason for hiding this comment

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

다른 분 리뷰에 민섭님이 남기신 걸 봤는데, 이 firebase 프로젝트는 리뷰가 끝나면 삭제하고 새로 만드시는 걸 추천드립니다. 이미 탈취되었을 가능성이 있고, 어떤식으로 악용 남용 될지 모르기 때문입니다.

Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

firbase config에 중요 정보들이 그대로 보여짐

변경 내용

변경한 내용
dotenv를 사용해서 api key들을 숨겼습니다.

해결 과정

지훈님의 코드 리뷰를 통해서 수정하였습니다.

리뷰 관련

보안이 정말 중요한데 dotenv로 환경 변수를 사용해서 앞으로 관리해야 할 것 같습니다. 이렇게 api key들을 관리하는 방법에는 dotenv가 가장 확실한 것인 지 궁금합니다!! 이번 부트 캠프 초반에 들었던 수업에서는 api key들을 github action의 api key를 관리하는 창에서 관리한다고 들었습니다. 그 외의 경우에는 대부분 dotenv를 사용해서 api key들을 숨기는 지 궁금합니다!!

Choose a reason for hiding this comment

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

Router와 component를 나눠서 관리했다면 더 좋았을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

발생 오류

Router기능, Store기능, Component기능이 다 index.js에 들어있어서 실행할 때마다 불필요한 코드들을 읽게됨

변경 내용

변경한 내용
index.js파일을 Component용으로 두고 Router.js, Store.js파일로 나누어 불필요한 코드가 실행되는 것을 방지

해결 과정

index.js 파일을 가현님의 피드백을 참고하여 각 기능 별로 나누었습니다.

리뷰 관련

index.js파일에 필요한 core 기능이 아닌 다른 기능들을 실행한다는 것을 미쳐 알지 못했습니다. 기능 별로 가장 작게 나누어야 불필요한 코드 진행을 막을 수 있기 때문에 이러한 점들을 계속해서 상기하며 코드를 짜야겠습니다.

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.

6 participants