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

14-7 [FE] [논문 상세 - 네트워크 차트] 노드 클릭 또는 hover 시 해당 노드와 자식 노드, 링크를 강조한다. #110

Merged
merged 9 commits into from
Dec 9, 2022

Conversation

Palwol
Copy link
Collaborator

@Palwol Palwol commented Dec 8, 2022

개요

그래프 노드를 클릭하거나 hover하면 해당 노드, 자식 노드(해당 노드의 reference 노드), 자식 노드와 연결된 링크가 밝게 강조됩니다.

불좀켜줄래

작업사항

  • 노드 클릭/hover 시 해당 노드 강조, 자식 노드, 연결된 링크 강조
  • zoom 제한 설정 (최소 0.1배, 최대 5배)
  • reference 목록 title에 태그 제거, 최대 길이 추가

리뷰 요청사항

usegraphEmphasize hook에서, 그래프에서 강조할 부분을 필터링해서 스타일을 적용하는 로직이 비슷하게 반복됩니다.
(클릭/호버된 노드면 강조 스타일, 아니면 기본 스타일)
반복되는 부분들을 함수로 분리할 수도 있을 것 같긴 한데, 이 hook 외에는 사용되지 않을 로직이라서 굳이 분리할 필요가 없을 것 같다는 생각도 듭니다. 다른 분들의 의견은 어떤지 궁금합니다!

Copy link
Collaborator

@JunYupK JunYupK left a comment

Choose a reason for hiding this comment

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

리뷰

리뷰 요청하신사항을 보고 해당 코드를 살펴보았는데, 적용되는 컴포넌트나 요소만 다를뿐 같은 로직이 반복되기는 합니다.
이를 함수로 분리해서 사용하면 더 깔끔하고 간결한 코드가 될 것 같긴 합니다.
우선 useGraphEmphasize.ts 해당 스크립트에서만 사용하는 함수로 빼서 사용해도 좋을 것 같습니다.
혹시나 추후에 비슷한 기능을 만들어야 할 경우가 생길 수도 있으니까요

Copy link
Member

@yeynii yeynii left a comment

Choose a reason for hiding this comment

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

아름다워요

frontend/src/hooks/graph/useGraphEmphasize.ts Outdated Show resolved Hide resolved
frontend/src/pages/PaperDetail/components/PaperInfo.tsx Outdated Show resolved Hide resolved
Comment on lines 29 to +30
useGraphZoom(svgRef.current);
useGraphHover(nodeRef.current, nodes, hoveredNode);
useGraphEmphasize(nodeRef.current, linkRef.current, nodes, links, hoveredNode, data.key);
Copy link
Member

Choose a reason for hiding this comment

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

나중에 graph 관련 Hook들을 모아서 useGraph 훅으로 정리해보면 좋을 것 같아요~

frontend/src/hooks/graph/useGraphEmphasize.ts Outdated Show resolved Hide resolved
@Palwol
Copy link
Collaborator Author

Palwol commented Dec 8, 2022

리뷰 요청하신사항을 보고 해당 코드를 살펴보았는데, 적용되는 컴포넌트나 요소만 다를뿐 같은 로직이 반복되기는 합니다.
이를 함수로 분리해서 사용하면 더 깔끔하고 간결한 코드가 될 것 같긴 합니다.
우선 useGraphEmphasize.ts 해당 스크립트에서만 사용하는 함수로 빼서 사용해도 좋을 것 같습니다.
혹시나 추후에 비슷한 기능을 만들어야 할 경우가 생길 수도 있으니까요

말씀하신대로 이 훅 안에서라도 함수로 분리해보면 좋을 것 같네요. 특히 link 강조 부분은 각 .style 부분마다 거의 똑같은 로직이 반복되고 있어서 분리해보겠습니다. 좋은 의견 감사합니다.

@yeynii
Copy link
Member

yeynii commented Dec 8, 2022

.on('click', (_, d) => d.doi && addChildrensNodes(d.doi));

충돌나는부분중에 click 이벤트는 dev쪽 로직으로,
그 외는 양쪽 변경사항 다 포함해서 컨플릭트 해결해주시면 될 것 같습니다!

@Palwol
Copy link
Collaborator Author

Palwol commented Dec 8, 2022

  • 리뷰사항 반영 완료했습니다.
  • highlightKeyword, sliceTitle 함수를 utils로 분리했습니다.
  • 노드 hover 시와 click 시 text 강조 효과를 다르게 적용하였습니다. 왼쪽 reference list hover 시 그래프에서 강조되는 효과가 더 잘 보이게 됩니다.

Comment on lines +24 to +26
export const sliceTitle = (title: string) => {
return title.length > MAX_TITLE_LENGTH ? `${title.slice(0, MAX_TITLE_LENGTH)}...` : title;
};

Choose a reason for hiding this comment

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

text-overflow: ellipsis css로도 처리할 수 있어요.

@Palwol Palwol merged commit da8e9c8 into dev Dec 9, 2022
@Palwol Palwol deleted the feature/graph-link-emphasize branch December 12, 2022 09:16
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.

4 participants