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(divider): Divider 컴포넌트 구현 #28

Merged
merged 8 commits into from
Nov 27, 2024
Merged

Conversation

heeji289
Copy link
Member

변경사항

  • 간단하게 구분선을 그리는 컴포넌트입니다.
  • color, size 등을 prop으로 뚫을까 하다가 구분선을 사용할 때 보통 margin, padding 등을 같이 쓰다보니 style을 받아서 사용자가 다 넣을 수 있게 했습니다. (radix와 MUI 인터페이스 참고)

시각자료

screenshot

체크리스트

  • 기능 명세를 작성하였나요?
  • 테스트 코드를 작성하였나요?

추가 논의사항

  • 제안주실 거나 이상한 점 모두 말해주세요!

@heeji289 heeji289 self-assigned this Nov 20, 2024
@heeji289 heeji289 linked an issue Nov 20, 2024 that may be closed by this pull request
@heeji289 heeji289 requested a review from synuns as a code owner November 20, 2024 06:35
@heeji289 heeji289 changed the title Divider 컴포넌트 구현 (#25 feat(divider): Divider 컴포넌트 구현 Nov 20, 2024

const divider = screen.getByRole('separator');

expect(divider).toHaveAttribute('aria-orientation', 'horizontal');
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 이런 aria 속성이 있는지 처음 알았네요~

const divider = screen.getByRole('separator');

expect(divider).toHaveAttribute('aria-orientation', 'horizontal');
expect(divider).toHaveClass(styles.horizontal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

class를 검증하기보다는 실제 적용되길 기대하는 스타일이 어떤건지를 검증해보면 어떨까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다! 228d398

@noahluftyang
Copy link
Collaborator

희지님 최고~~ 👍👍👍

return (
<hr
aria-orientation={orientation}
className={`${styles[orientation]} ${styles.divider}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

현재 props에 className이 없어서 TailwindCSS를 사용하는 프로젝트에서 스타일을 확장하거나 커스터마이징하기 어려울 것 같습니다.
컴포넌트를 여러 프로젝트에서 재사용하려면 className을 지원해 스타일링의 유연성을 높이는 것이 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

a198ac8 수정했습니다!
놓쳤던 부분인데 감사합니다

Copy link
Member

@hy57in hy57in left a comment

Choose a reason for hiding this comment

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

LGTM!!!!!

@hy57in
Copy link
Member

hy57in commented Nov 23, 2024

오호 radix와 MUI가 style 로 받게 되어 있군요..!
이것도 논의해보고 싶은 주제인데 Divider 컴포넌트에 margin, padding을 넣는것을 선호하는지 Divider 를 사용하는 곳에서 margin, padding 간격을 주입하는 것을 선호하는지 궁금합니다!!
실제로 회사에서 Divider 만들 때 margin 을 만들지 말지에 대해 치열하게 논의했던 적이 있어서요 ㅎㅎ

@hy57in
Copy link
Member

hy57in commented Nov 23, 2024

테스트!

@heeji289
Copy link
Member Author

  • forwardRef
  • props 넘기기

@noahluftyang
Copy link
Collaborator

오호 radix와 MUI가 style 로 받게 되어 있군요..! 이것도 논의해보고 싶은 주제인데 Divider 컴포넌트에 margin, padding을 넣는것을 선호하는지 Divider 를 사용하는 곳에서 margin, padding 간격을 주입하는 것을 선호하는지 궁금합니다!! 실제로 회사에서 Divider 만들 때 margin 을 만들지 말지에 대해 치열하게 논의했던 적이 있어서요 ㅎㅎ

이거 다른 분들 의견도 궁금한데 채널에서 함 논의해보면 어떨까요??

@synuns
Copy link
Member

synuns commented Nov 26, 2024

오호 radix와 MUI가 style 로 받게 되어 있군요..! 이것도 논의해보고 싶은 주제인데 Divider 컴포넌트에 margin, padding을 넣는것을 선호하는지 Divider 를 사용하는 곳에서 margin, padding 간격을 주입하는 것을 선호하는지 궁금합니다!! 실제로 회사에서 Divider 만들 때 margin 을 만들지 말지에 대해 치열하게 논의했던 적이 있어서요 ㅎㅎ

이거 다른 분들 의견도 궁금한데 채널에서 함 논의해보면 어떨까요??

좋아요! 다음 안건으로 올려요!!

Copy link

changeset-bot bot commented Nov 27, 2024

⚠️ No Changeset found

Latest commit: a198ac8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@heeji289 heeji289 requested a review from froggy1014 November 27, 2024 04:34
@noahluftyang noahluftyang merged commit e3a1a7d into main Nov 27, 2024
1 check passed
@noahluftyang noahluftyang deleted the feat/divider branch November 27, 2024 12:03
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.

Divider
5 participants