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

Develop/smooth scrolling #31

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

Develop/smooth scrolling #31

wants to merge 3 commits into from

Conversation

nnoonjy
Copy link
Contributor

@nnoonjy nnoonjy commented Sep 17, 2024

리팩토링 🛠

smooth scroll 구현

@nnoonjy nnoonjy linked an issue Sep 17, 2024 that may be closed by this pull request
@cla6shade
Copy link
Member

우선 eslint 문제만 해결해서 푸쉬해뒀습니다.

다만 useSmoothScroll을 만드실 때 use 키워드를 사용해서 훅을 만드셨는데, 추후에 ref Object과의 연결 등을 추가로 지원하기 위해서 만드신 걸까요?

현재는 react 컴포넌트의 생명 주기와 상관이 없어 hook의 규칙을 따를 필요가 없고, 아래 사용 용례는 훅 사용 규칙에 어긋납니다.

  const scrollToSection = (sectionId: string) => {
    const section = document.getElementById(sectionId);
    if (section) {
      const targetPosition = section.getBoundingClientRect().top + window.scrollY;
      useSmoothScroll(targetPosition);
    }
  };

hook처럼 동작하도록 하고싶으시다면 ref object와 연결해서 만들고, 훅의 규칙을 따라 실행되도록 리팩터링이 필요할 것 같습니다! 그리고 현재 상태 그대로 일반 함수처럼 작동하도록 하고 싶으시다면 @utils/ 하위로 옮기는 것이 좋을 것 같아요.

어떻게 생각하시나요?

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.

Smooth scrolling
2 participants