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

feat(view): add transition in ClusterGraph #142

Merged
merged 4 commits into from
Sep 12, 2022
Merged

feat(view): add transition in ClusterGraph #142

merged 4 commits into from
Sep 12, 2022

Conversation

vgihan
Copy link
Contributor

@vgihan vgihan commented Sep 11, 2022

WorkList

b9cc3a4

  • selectedData 변경 시 Detail 컴포넌트가 보여질 때 ClusterGraph에 transition 추가했습니다.
  • 컴포넌트 렌더링시에 svg가 매번 리렌더링 되기 때문에, transition을 적용하기 위해선 이전 selectedIndex 값을 기억하고 있어야 합니다.
  • 그렇기 때문에 useRef를 통해 이전 selected 값을 기억하도록 하고, d3의 draw 작업이 마무리되면 현재 selected 값을 selectedPrev에 저장합니다.
  • 이후 해당 값을 data로 넘겨 초기 위치는 이전 selected를 기반으로, 이어지는 transition은 현재 selected 값을 기준으로 발생하도록 구현했습니다.

0ea3fc7

  • ClusterGraph 의 className들을 컨벤션에 맞게 수정했습니다. (ex. cluster-graph_container)

27ae411

  • summary 컴포넌트에 Detail이 펼쳐지는 transition을 적용했습니다.
  • Detail의 open transition과 close transition이 동시에 일어날 때, ClusterGraph와 싱크가 맞지 않는 문제가 있기 때문에 clusterGraph와 Summary의 close transition을 삭제했습니다.

7466762

  • detail 컴포넌트가 펼쳐질 때, ClusterGraph height가 증가하지 않아 아랫부분이 잘려나가는 문제를 해결했습니다.

Result

@vgihan vgihan self-assigned this Sep 11, 2022
@vgihan vgihan requested a review from a team as a code owner September 11, 2022 19:27
Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

와우! githru prototype만들 때, transition 부분 꼭 넣어보고 싶었는데 드디어 기능이 들어가는군요 :- )
전체적으로 잘 적용이 된다면 애플리케이션 전체적으로 유려한 느낌을 줄 수 있을 것 같습니다!

추후에 전반적인 app에서 사용하는 원칙(?)같은 것(ex. transition의 시간 등)을 rule로 만들어도 좋을 것 같네요!!

@@ -1,18 +1,18 @@
.cluster-container {
.cluster-box {
.cluster-graph_container {
Copy link
Contributor

Choose a reason for hiding this comment

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

변수 naming부분은 이전 리뷰들을 보니까 view팀에서 정의하신 부분들이 있는 것 같아요.
@githru/view 팀분들 확인부탁드립니다~.

return `translate(${x}, ${y})`;
}

export function getSelectedIndex(
Copy link
Contributor

Choose a reason for hiding this comment

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

(궁금) 혹시 selectedIndex를 다시 전체 data에서 찾는 구조인가요?
만약 그렇다면 추후에 component간 주고 받는 데이터에 selectedIndex를 넣어줘도 괜찮을 것 같습니다~.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞습니다...! 전체 데이터에서 selected 된 데이터를 찾는 구조인데, 추후에 selectedIndex를 추가해서 한 번더 탐색하는 작업을 줄일 수 있을 것 같습니다 :)

@jin-Pro
Copy link
Member

jin-Pro commented Sep 12, 2022

LGTM~~~ 수고하셨어요 ㅎㅎ

@vgihan
Copy link
Contributor Author

vgihan commented Sep 12, 2022

후속 작업을 위해 우선 merge 하겠습니다 :)

@vgihan vgihan merged commit be0a8d6 into githru:main Sep 12, 2022
@hanseul-lee hanseul-lee added this to the v0.1.0 milestone Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants