Skip to content

Link 컴포넌트를 웹사이트에 적용#288

Merged
RiaOh merged 5 commits intomainfrom
203-apply-link-component-to-website
May 31, 2025
Merged

Link 컴포넌트를 웹사이트에 적용#288
RiaOh merged 5 commits intomainfrom
203-apply-link-component-to-website

Conversation

@RiaOh
Copy link
Contributor

@RiaOh RiaOh commented May 29, 2025 •
edited
Loading

변경 사항

  • 무엇을 변경했나요? (예: 새 컴포넌트 추가, 디자인 수정, 문서 업데이트)
    • daleui.com 웹사이트에 있는 요소를 Link 컴포넌트로 변경
    • Link 컴포넌트의 href 속성을 필수속성으로 추가

목적

  • 왜 이 변경이 필요한가요? (예: 접근성 개선, 사용자 경험 향상)
    • 우리 마케팅 웹사이트에 구현해놓은 Link 컴포넌트 적용 필요
    • a tag에는 href 속성이 필수

리뷰어에게

  • 특별히 확인해 주셨으면 하는 부분이 있나요? (예: 호버 상태, 모바일 뷰포트)
    • 작업을 진행하면서 children 속성도 필수 속성으로 지정해야하나? 하는 생각이 들었습니다.
      확인해보니 PropsWithChildren은 children 속성을 컴포넌트의 props에 추가는 하지만 필수임을 보장하지 않는다고 하네요!
      a태그에 children이 기술적?으로는 필수는 아니지만.. children이 없는 경우가 있는지.. 의견 궁금합니다 :)
    • href속성을 필수로 지정하면서 Homepage.stories.tsx 파일에 사용된 Link 컴포넌트에 href 속성을 추가했습니다. href속성으로는 임시로 '#'값을 주어 iframe url로 이동되는 버그가 있지만, Link 컴포넌트 스토리북 버그 #215 작업 시 같이 수정할 예정입니다!

@RiaOh RiaOh requested a review from a team as a code owner May 29, 2025 13:29
@RiaOh RiaOh linked an issue May 29, 2025 that may be closed by this pull request
Link 컴포넌트를 웹사이트에 적용 #203
Closed
DaleSeo
DaleSeo reviewed May 29, 2025
View reviewed changes
Copy link
Member

@DaleSeo DaleSeo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

타입 오류가 발생하고 있습니다. 리뷰 요청하시기 전에 CI가 실패하는지 확인하는 습관을 들이시면 좋습니다. 😃

image

https://github.com/DaleStudy/daleui/actions/runs/15324991718/job/43117142288?pr=288

@RiaOh
Copy link
Contributor Author

RiaOh commented May 30, 2025

@DaleSeo 앗.. 감사합니다. 실패하는지 확인못했네요!ㅠ
수정하고 커밋 push했습니다! :)

@DaleSeo
Copy link
Member

DaleSeo commented May 30, 2025

refactor: change a tag to Link component

PR 제목은 합의했던 것처럼 가급적 한국어로 작성해주시면 감사하겠습니다. 이슈 제목과 PR 제목을 맞춰주시면 나중에 찾기가 수월해서 추천드립니니다. 관련해서 컨벤션 문서 리마인드 드립니다: https://github.com/DaleStudy/daleui/wiki/Conventions

@DaleSeo
Copy link
Member

DaleSeo commented May 30, 2025

작업을 진행하면서 children 속성도 필수 속성으로 지정해야하나? 하는 생각이 들었습니다.
확인해보니 PropsWithChildren은 children 속성을 컴포넌트의 props에 추가는 하지만 필수임을 보장하지 않는다고 하네요!
a태그에 children이 기술적?으로는 필수는 아니지만.. children이 없는 경우가 있는지.. 의견 궁금합니다 :)

이 부분 원 설계자이신 @hyoseong1994 님과 의논하시면 좋을 것 같습니다.

DaleSeo
DaleSeo approved these changes May 30, 2025
View reviewed changes
Copy link
Member

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

사소한 피드백과 함께 승인합니다. 수고하셨습니다.

src/App.tsx
import { Button } from "./components/Button/Button";
import { Heading } from "./components/Heading/Heading";
import { Text } from "./components/Text/Text";
import { Link } from "./components/Link/Link";
Copy link
Member

@DaleSeo DaleSeo May 30, 2025

Choose a reason for hiding this comment

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

import 문을 알파벳 순으로 정렬해놓으면 import 문이 많아졌을 때 읽기가 편해집니다.

Copy link
Contributor Author

@RiaOh RiaOh May 31, 2025

Choose a reason for hiding this comment

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

오.. 좋은 피드백 감사합니다! 반영하겠습니다 :)

@RiaOh RiaOh changed the title refactor: change a tag to Link component Link 컴포넌트를 웹사이트에 적용 May 30, 2025
@RiaOh
Copy link
Contributor Author

RiaOh commented May 30, 2025

작업을 진행하면서 children 속성도 필수 속성으로 지정해야하나? 하는 생각이 들었습니다.
확인해보니 PropsWithChildren은 children 속성을 컴포넌트의 props에 추가는 하지만 필수임을 보장하지 않는다고 하네요!
a태그에 children이 기술적?으로는 필수는 아니지만.. children이 없는 경우가 있는지.. 의견 궁금합니다 :)

이 부분 원 설계자이신 @hyoseong1994 님과 의논하시면 좋을 것 같습니다.

@hyoseong1994 님!
이 부분 논의가 필요할 것 같아서요~!
혹시 보시고 피드백 부탁드립니다 :)

@hyoseong1994
Copy link
Contributor

hyoseong1994 commented May 30, 2025

제가 생각하기에도 children을 필수로 변경하는게 더 올바른 방향같습니다. 당시 설계때 @Sunjae95 님과 @RiaOh 님 3명이서 설계를 진행했는데 다들 동의하실까요?

@Sunjae95
Copy link
Member

Sunjae95 commented May 30, 2025

children 필수로 받는거 좋은 의견인데요? 저도 동감입니다!

Sunjae95
Sunjae95 approved these changes May 30, 2025
View reviewed changes
Copy link
Member

@Sunjae95 Sunjae95 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

@RiaOh
Copy link
Contributor Author

RiaOh commented May 31, 2025

@hyoseong1994 @Sunjae95 저도 children을 필수속성으로 지정하는게 좋은 방향인 것 같아요!
수정해서 올려놓겠습니다! 감사합니다~! :)

@RiaOh RiaOh merged commit 91747cc into main May 31, 2025
9 checks passed
@RiaOh RiaOh deleted the 203-apply-link-component-to-website branch May 31, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaleSeo DaleSeo DaleSeo approved these changes

+1 more reviewer

@Sunjae95 Sunjae95 Sunjae95 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RiaOh RiaOh

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link 컴포넌트를 웹사이트에 적용

4 participants

@RiaOh @DaleSeo @hyoseong1994 @Sunjae95