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

Comment Like / Declaration 기능 추가 #20

Merged
merged 24 commits into from
Mar 19, 2020

Conversation

SoYoung210
Copy link
Contributor

Desc

useGesture 중 useDrag를 사용하여 사용자의 인터랙션에 따른 동작을 구현했습니다.

Demo

demo

Reference

@SoYoung210 SoYoung210 requested a review from JaeYeopHan as a code owner March 9, 2020 04:16
@SoYoung210 SoYoung210 self-assigned this Mar 9, 2020
@vercel
Copy link

vercel bot commented Mar 9, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/zpapoo/ncha-frontend/qy4r9m5kf
✅ Preview: https://ncha-frontend-git-feature-toggle-comment-menu.zpapoo.now.sh

@SoYoung210 SoYoung210 changed the base branch from feature/timeline-drag-drop to develop March 9, 2020 05:14
Copy link
Member

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

hooks/

  • useFetch.ts
  • use-*.ts

이렇게 하는 것이 좋을 것 같은데, 어떠신가요?

useEffect(() => {
if (mouseRef.current) {
const { offsetWidth } = mouseRef.current
const percent = Math.min(Math.max(diff / offsetWidth, 0), 1)
Copy link
Member

Choose a reason for hiding this comment

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

how about this:

const percent = getBoundedValue(diff / offsetWidth)

function getBoundedValue(val: number, customOption?: { min: number; max: number }) {
  const option = { ...{ min: 0, max: 1 }, ...customOption } // need to refactoring
  return Math.min(Math.max(val, min), max)
}

i think utils means...

  1. 비즈니스 로직과 상관없는 단순 계산 함수들
  2. 다른 부분에서도 사용될 수 있는 공통 로직을 다루는 함수들

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 굉장히 nit한 질문인데, function functionName 으로 사용하시는 이유가 있나욤? 저는 all cosnt functionName = () => {}로 사용하고 있어서 ㅎㅎ 혹시 뭔가 이유가 있나 궁금합니돵

Copy link
Member

Choose a reason for hiding this comment

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

오호 괜춘한 질문!

리터럴 함수 정의(1) vs Arrow Function으로 함수 정의(2)

Screen Shot 2020-03-17 at 9 48 28 AM

자동 완성 시, (1)에 대해서는 함수로 나오는데 (2)에 대해선 변수로 나옴.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 핵꿀팁.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다음 PR에서 파일이름이랑 정리할 수 있으면 한번 해볼게용~

@SoYoung210
Copy link
Contributor Author

@JaeYeopHan 같이 리팩토링 했던 유틸함수 날려먹은것을 지금알아서.. 복구했습니다.. this commit

@JaeYeopHan
Copy link
Member

hooks/

  • useFetch.ts
  • use-*.ts

이렇게 하는 것이 좋을 것 같은데, 어떠신가요?

이건 왜 노응답?

@SoYoung210
Copy link
Contributor Author

hooks/

  • useFetch.ts
  • use-*.ts

이렇게 하는 것이 좋을 것 같은데, 어떠신가요?

이건 왜 노응답?

Internal soso Error.... (놓치고 처리하지 못함)
좋아용 내일 반영하고 리뷰 재요청 할게요~

@SoYoung210
Copy link
Contributor Author

@JaeYeopHan hooks파일명에 대한 생각이 바뀌었습니다. ㅎㅎ

hooks라는 폴더 이름으로 인해, 이미 custom hooks가 있을것이 정해졌는데 앞에 use를 붙여주는 것은 파일명을 늘리는 것은 아닐지 염려됩니당.

hooks이야기가 나와서 전체 폴더를 돌아보니..
image

어떤 파일은 폴더의 성격까지 나타내고, 어떤 파일은 그렇지 않은데요, 이 PR에서 논의하고 별도 PR로 네이밍 정리하는 PR진행하겠습니다.

저는 서두에 말씀드린 내용으로, 파일은 폴더의 성격을 나타내지 않는것을 제안드립니다.

@JaeYeopHan
Copy link
Member

이 부분에 대한 것은 의견만 드리자면

파일명이 겹치는 것을 최대한 피하려고 하는 편입니다. index를 최대한 지양하는 것도 이와 동일한 이유에서 인데요, 예를 들어

utils/common.tsconstants/common.ts인 경우를 피하고자 Utils, Constants를 붙입니다.
hooks 디렉토리에도 fetch, swiper가 있는데 파일명 만으로 이것이 hooks인지 아닌지 알 수 있어야 한다고 생각합니다.

다만 이 이야기를 길게 하고 싶지 않아요~ 프로젝트 별로 정해면 되는 부분이니, 소영님이 끌리는대로 다음 PR에서 정리해주세요~

@SoYoung210
Copy link
Contributor Author

@JaeYeopHan 곰곰2 생각해보니 재엽님 의견에 좀더 동의되네요! 그냥 의미론 적인것을 맞추기위해서 보다는 개발자 편의성측면에서 동의합니닷!@ 이 PR은 아직 안어프룹인가요..?

Copy link
Member

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

💯

@JaeYeopHan JaeYeopHan merged commit b634d59 into develop Mar 19, 2020
@JaeYeopHan JaeYeopHan deleted the feature/toggle-comment-menu branch March 19, 2020 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants