eastjun-dev / frontend

MIT License
2 stars 4 forks source link

[mission001] todolist crud #32

Open amorfati0310 opened 4 years ago

amorfati0310 commented 4 years ago

다 못하였는데 한달쨰 PR 안 날리고 있어서 부끄러워서 일단 한데 까지만 이라도 날리고 이어서 작업하도록 하겠습니다 ㅜ

아쉬운 점

  1. class를 사용했는데 너무 this bind가 남용되고 있는 것 같습니다.
  2. 상수로 뺄 수 있는 부분 빼지 못한 점
  3. 뷰 모델 연결관계 구조 및 네이밍 vue 에 Dep와 비슷한 구조와 + extends를 활용하고 dispatch action 등 네이밍만 비슷하게 사용한게 아닌지 어색한 느낌이 강하네요
  4. 웹팩 설정을 하다가 말았네요 다음스텝 넘어가면서 더 추가해보겠습니다 _ㅇ

특히 더 리뷰 받고 싶은 부분

ETC

ganeodolu commented 4 years ago

다시 만나게 되어서 반가워요. 저도 많이 밀리다보면 쫓아가기 버겁고, 그러다가 보면 놓아버리는 경우가 생기더라구요. 계속 함께 발전해 나갔으면 좋겠습니다. :smiley: index.html을 열어보면 작동을 하지 않는데 혹시 script로 js파일을 불러오는 부분은 필요 없는지요?? 그래서 app.js에서 css도 불러오지를 못하는 것 같아요.

HoseokNa commented 4 years ago

안녕하세요! 늦었지만 코멘트 달아봅니다. 리뷰어는 아니지만 소감과 몇가지 조심스럽게 피드백 남기겠습니다.

먼저 전체적인 느낌을 말씀드리겠습니다.

  1. 파일이 세부적으로 나누어져 있어서 보기 좋았습니다. (api, component, store, util 등)

  2. dom을 사용하는 함수들을 dom.js로 관리하는게 인상 싶었습니다.

  3. vue를 사용해 보지 않았었는데 어떤 느낌인지 간접적으로 알 수 있어서 좋았습니다.

다음은 피드백 내용인데 아직 많이 부족해서 작은 부분만 해드릴 수 있는 점 양해 부탁드리겠습니다.

  1. vue를 사용하지 않아서 네이밍과 구조에 대해서 피드백 드리기가 조심스럽네요. 이 부분은 다른 분들의 조언을 구하겠습니다.

  2. 전체적으로 공백이 여러개 들어가 있는 파일이 있습니다. 혹은 공백이 없거나. (ex import 다음, export 전) 이 부분은 통일 시키면 보기 더 좋을 것 같습니다.

  3. api의 index.js에는 destructing을 사용 됐는데 다른 곳에도 적용되면 좋을 것 같습니다. 아니면 혹시 다른 이유가 있는 것인지 궁금합니다. const id = getClosetLI(target).dataset.id { id } = getClosetLI(target).dataset

  4. switch 문에 주석이 달린 부분이 있는데 좀 더 구체적인 증상이 궁금합니다. break도 사용 안했을 때 case를 모두 들어가게 되는 말씀인지 궁금하네요. 문자열 비교이기 때문에 상관 없을 것 같은데 이상한 현상이네요. 증상 재현을 해보려고 했는데 아직 환경 구성에 능숙하지 못해서 구동을 못했습니다.(흑흑)

amorfati0310 commented 4 years ago

다시 만나게 되어서 반가워요. 저도 많이 밀리다보면 쫓아가기 버겁고, 그러다가 보면 놓아버리는 경우가 생기더라구요. 계속 함께 발전해 나갔으면 좋겠습니다. 😃 격려 너무 감사합니다 ㅜ .. 요즘 멘탈 번 아웃이 왔었나보네요 ... index.html을 열어보면 작동을 하지 않는데 혹시 script로 js파일을 불러오는 부분은 필요 없는지요??

  • 아 실행이 되려면 package.json에 webpack 을 실행시켜야 보이는데 좀 더 테스트 환경 개선할 수 있게 적용해볼게요 감사합니다 ~~ :) 그래서 app.js에서 css도 불러오지를 못하는 것 같아요.
amorfati0310 commented 4 years ago

안녕하세요! 늦었지만 코멘트 달아봅니다. 리뷰어는 아니지만 소감과 몇가지 조심스럽게 피드백 남기겠습니다.

먼저 전체적인 느낌을 말씀드리겠습니다.

  1. 파일이 세부적으로 나누어져 있어서 보기 좋았습니다. (api, component, store, util 등)
  2. dom을 사용하는 함수들을 dom.js로 관리하는게 인상 싶었습니다.
  3. vue를 사용해 보지 않았었는데 어떤 느낌인지 간접적으로 알 수 있어서 좋았습니다.

좋게봐주셔서 감사합니다 ㅜㅜ vue에 dep만 가지고 온 부분이여서 이상하게 짬뽕시켰을 확률이 높아요

다음은 피드백 내용인데 아직 많이 부족해서 작은 부분만 해드릴 수 있는 점 양해 부탁드리겠습니다. 아닙니다 저도 마찬가지입니다

  1. vue를 사용하지 않아서 네이밍과 구조에 대해서 피드백 드리기가 조심스럽네요. 이 부분은 다른 분들의 조언을 구하겠습니다. 아닙니다 적극적으로 의견 주시면 감사하겠습니다 ~~ :)
  2. 전체적으로 공백이 여러개 들어가 있는 파일이 있습니다. 혹은 공백이 없거나. (ex import 다음, export 전) 이 부분은 통일 시키면 보기 더 좋을 것 같습니다. 감사합니다 신경써볼게요
  3. api의 index.js에는 destructing을 사용 됐는데 다른 곳에도 적용되면 좋을 것 같습니다. 아니면 혹시 다른 이유가 있는 것인지 궁금합니다. const id = getClosetLI(target).dataset.id { id } = getClosetLI(target).dataset 이게 더 좋을 것 같네요 적용할게요 감사합니다
  4. switch 문에 주석이 달린 부분이 있는데 좀 더 구체적인 증상이 궁금합니다. break도 사용 안했을 때 case를 모두 들어가게 되는 말씀인지 궁금하네요. 문자열 비교이기 때문에 상관 없을 것 같은데 이상한 현상이네요. 증상 재현을 해보려고 했는데 아직 환경 구성에 능숙하지 못해서 구동을 못했습니다.(흑흑) 다시 한 번 살펴볼게요 감사합니다 ~~
amorfati0310 commented 4 years ago

일요일에 마저 안 된 부분 진행하고 피드백 요청 드릴게유 ~~