Skip to content

refactor(view): processMessage 함수 내 GIthubIssueLink 컴포넌트 분리#921

Merged
nxnaxx merged 2 commits into
githru:mainfrom
nxnaxx:refactor/#900_extract-issue-link-component
Oct 6, 2025
Merged

refactor(view): processMessage 함수 내 GIthubIssueLink 컴포넌트 분리#921
nxnaxx merged 2 commits into
githru:mainfrom
nxnaxx:refactor/#900_extract-issue-link-component

Conversation

@nxnaxx

@nxnaxx nxnaxx commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Related issue

#900

Result

Detail과 Content 컴포넌트의 processMessage 함수 내 중복으로 존재하던 Github 이슈 링크 JSX 로직을 컴포넌트화하였습니다.

Work list

  • 공통 컴포넌트 생성: GitHub 이슈 링크 생성 로직 캡슐화
  • 기존 컴포넌트 리팩토링: Detail.tsx, Content.tsx 내 인라인 JSX를 GithubIssueLink 컴포넌트로 교체
  • 스타일 중복 제거: 이슈 링크 관련 CSS 스타일을 GithubIssueLink.scss로 통합
  • 코드 스타일: Detail.scss 내 불필요한 주석 제거

Test

  • 기존 GitHub 이슈 링크 기능이 정상 동작하는지 확인(processMessage)
  • GithubIssueLink 컴포넌트가 올바른 URL을 생성하는지 확인
  • CSS 스타일이 기존과 동일하게 적용되는지 확인

후속 작업

  • processMessage 함수 내 공통 로직 분리 / 테스트
  • 이슈 링크 () 괄호 패턴 매칭 안되는 오류 수정

Discussion

디버깅 모드에서 아래와 같이 Merge PR와 연관된 커밋들이 각 커밋 메시지가 아닌 동일한 PR 제목/본문으로 표시됩니다.
스크린샷 2025-10-01 오후 6 22 08

  1. 만약 의도된 동작이라면, 연관 커밋마다 동일한 PR 본문을 중복 노출할 필요성이 있는가에 대한 의문이 있습니다.
  2. 의도된 동작이 아니라면 아래와 같이 Merge 커밋에만 PR 제목/본문을 노출하고, 연관 커밋은 해당 커밋의 제목/본문을 노출하는 것이 적절해 보입니다.
스크린샷 2025-10-01 오후 6 11 36 스크린샷 2025-10-01 오후 6 11 41

nxnaxx added 2 commits October 1, 2025 20:30
… in Detail/Content

- Replace inline issue link JSX in Detail/Content with <GithubIssueLink />
- Deduplicate GitHub issue link styles
@@ -0,0 +1,21 @@
@import "styles/app";

.commit-message__issue-link {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

의견입니다ㅎㅎ

이제 이름이 생겼으니 스타일에서도 .github-issue-link 이런 느낌으로 쓰는거 어떨까요

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

의견 감사합니다!!
분리에만 집중하다보니 놓쳤네요. 저도 컴포넌트명이랑 통일시키는 게 좋은 것 같습니다 ㅎㅎ 다음 작업 때 반영해 보겠습니다😄

@ytaek ytaek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

항상 신경쓰였던 부분인데, 공통부 분리가 되서 넘 좋습니다!!

@@ -0,0 +1,8 @@
import type { ComponentPropsWithoutRef } from "react";

export interface GithubIssueLinkProps

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

오오!! 고급 type 활용편인가요!!! 👍👍👍

@nxnaxx nxnaxx Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

공통 컴포넌트로 추출하다보니 지금 사용하고 있는 두 컴포넌트 이외에도 추후에 다른 곳에서 사용될 수 있을 것 같아서 <a> 기본 속성들을 자동으로 상속해주도록 만들었습니다!


import { Author } from "components/@common/Author";
import { useGithubInfo, useDataStore } from "store";
import { Author } from "components/@common/Author";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(취향, 개취), common 폴더 앞에 @골뱅이도 나중에 없애면 좋긴 하겠습니다. 이전에 common이 상단에 오는게 좋다면서 @를 붙이자고 하셨는데, 여전히 개인적으로는 뭔가 거슬리네요 😈😈😈😈😈

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

왜 해당 폴더만 @가 붙어있는지 궁금했었는데 그런 히스토리가 있었군요..!
저도 개인적으로는 변경하는 것이 좋다고 생각합니다. path alias에 주로 사용되는 접두어라 헷갈리기도 하구요

단순히 @만 제거하고 components/common/으로 남기기에는 다른 컴포넌트 폴더와 다르게 소문자라 컨벤션이 안 맞고, PascalCase로 변경하기에는 단일 컴포넌트로 오해할 수 있을 것 같은데요.
src/ 하위로 빼서 src/shared/ui/src/common/ 으로 두는 것은 어떨지 의견을 듣고 싶습니다!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

단순히 @만 제거하고 components/common/으로 남기기에는 다른 컴포넌트 폴더와 다르게 소문자라 컨벤션이 안 맞고, PascalCase로 변경하기에는 단일 컴포넌트로 오해할 수 있을 것 같은데요.
src/ 하위로 빼서 src/shared/ui/src/common/ 으로 두는 것은 어떨지 의견을 듣고 싶습니다!

@nxnaxx 님, 요 부분 issue로 discussion label 달고 만드시면 어떨까요?
살짝 얘기해보고 의견 받아서 반영하면 좋겠습니다.
(저는 개인적으로는 대 찬성입니다!!)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

네 이슈로 올려보겠습니다😄

@nxnaxx nxnaxx self-assigned this Oct 2, 2025
@nxnaxx
nxnaxx merged commit a7b2ede into githru:main Oct 6, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants