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

[view] 커밋 링크 추가 #668

Merged
merged 1 commit into from
Aug 27, 2024
Merged

[view] 커밋 링크 추가 #668

merged 1 commit into from
Aug 27, 2024

Conversation

GBAJS754
Copy link
Contributor

@GBAJS754 GBAJS754 commented Aug 25, 2024

Related issue

#534

Result

2024-08-25.6.37.01.mov

Work list

  • 커밋 해시 코드를 클릭하면 해당 커밋 내역 링크 페이지로 이동합니다.

Discussion

  1. 현재 keydown시 커밋 해시 코드가 복사되고 있는데요. 모든 키보드 입력을 처리하기보다는 Enter 키를 눌렀을 때만 커밋 해시 코드가 복사되도록 하는게 더 자연스러울것같은데 어떻게 생각하시나요?👀 추가로 불필요한 handleCommitIdCopy함수 실행도 줄일수있을것같아요! :)
  2. 오픈소스에 기여하는 작업이 익숙하지 않아 문제가 있다면 알려주세요! 감사합니다😌

@GBAJS754 GBAJS754 self-assigned this Aug 25, 2024
@GBAJS754 GBAJS754 requested review from a team as code owners August 25, 2024 09:57
@GBAJS754 GBAJS754 requested a review from ytaek August 25, 2024 09:59
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.

LGTM!! PR 올리시느라 고생하셨습니다!!! 👍👍👍
기능 자체의 확장에 대해서도 좀 고민하면 좋을 것 같아서 해당 내용 comment 남깁니다.

@@ -84,15 +85,15 @@ const Detail = ({ selectedData, clusterId, authSrcMap }: DetailProps) => {
</span>
</div>
<div className="commit-id">
<span
<a
href={`https://github.com/${owner}/${repo}/commit/${id}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 기능에 추가로, github.com 외의 도메인도 지원하게 하자는 #465 이슈도 추후에 고려해주셨으면 좋겠습니다~~
(enterprise github등을 쓰는 경우에는 github.com 이 아닌 다른 도메인도 쓰인답니다!)

onClick={handleCommitIdCopy(id)}
role="button"
tabIndex={0}
onKeyDown={handleCommitIdCopy(id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

onkeydown 은 저도 왜 있는지 모르겠네요 ^^;;

@ytaek
Copy link
Contributor

ytaek commented Aug 25, 2024

  • 커밋 해시 코드를 클릭하면 해당 커밋 내역 링크 페이지로 이동합니다.
  1. 현재 keydown시 커밋 해시 코드가 복사되고 있는데요. 모든 키보드 입력을 처리하기보다는 Enter 키를 눌렀을 때만 커밋 해시 코드가 복사되도록 하는게 더 자연스러울것같은데 어떻게 생각하시나요?👀 추가로 불필요한 handleCommitIdCopy함수 실행도 줄일수있을것같아요! :)

(말씀해주신걸 보니, 이제야 기억이 났네요;; 해당 기능은 git-graph에 있는 것처럼, commit ID를 눌리면 hash 값이 복사되어서, checkout할 때 등등 사용할 수 있도록 하는 기능이었습니다)
진행해주신 부분에서 추후에는 몇가지 고려해야될 점이 있을 것 같아요.

  • 현재는 외부 브라우저로 바로 링크가 열리는 것 같은데(맞죠?), vscode의 다른 panel로 열리거나, webview안에서 작은 frame으로 열리거나 하게 할 수도 있을 것 같아요.
  • 혹은 이런 외부 링크 열리는 방법들을 사용자 셋팅으로 결정되게 할 수도 있구요.
  • 그리고, 리뷰에 남긴 것처럼, github.com 외의 도메인을 지원하는 부분을 고려해야 할 수도 있을 것 같습니다.

위 사항들 잘게 쪼개서 이슈로 등록해주시면 좋을 것 같고,
view팀과 같이 얘기 나눠봐도 좋을 것 같습니다 (@seungineer)

  1. 오픈소스에 기여하는 작업이 익숙하지 않아 문제가 있다면 알려주세요! 감사합니다😌
  • 잘 진행하고 있으신 것 같아요!! 추가 활동들도 기대하겠습니다!! 💯💯💯💯💯

Copy link
Contributor

@shgusgh12 shgusgh12 left a comment

Choose a reason for hiding this comment

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

LGTM 고생하셨습니다~!!

Copy link
Contributor

@joonwonBaek joonwonBaek left a comment

Choose a reason for hiding this comment

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

LGTM 고생하셨습니다 ㅎㅎ

@GBAJS754
Copy link
Contributor Author

리뷰 감사합니다!ㅎㅎ말씀주신 부분들 잘 정리해서 이슈로 등록해둘게요!!

@GBAJS754 GBAJS754 merged commit fe39a1c into githru:main Aug 27, 2024
2 checks passed
@seungineer seungineer added this to the v0.7.1 milestone Aug 31, 2024
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.

5 participants