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

[Feature] Text 컴포넌트 구현 #18

Merged
merged 6 commits into from
Aug 14, 2024
Merged

[Feature] Text 컴포넌트 구현 #18

merged 6 commits into from
Aug 14, 2024

Conversation

SeieunYoo
Copy link
Collaborator

@SeieunYoo SeieunYoo commented Aug 12, 2024

🎉 변경 사항

  • Text 컴포넌트 구현합니다.

🚩 관련 이슈

🙏 여기는 꼭 봐주세요!

  • staticCss 에서 textStyle 을 추가해야 모든 textStyle css 가 정의되어서 프로퍼티로 넘긴 타이포 토큰에 따라 동적으로 대응됐습니다.
  • 아래처럼 쓸 수 있어서 텍스트 컴포넌트를 굳이 만들어야 하나,,?! 라는 생각이 들었지만 디폴트로 타이포와 컬러를 지정하면 편하게 쓸 수 있지 않을까 하여 피알 올려봅니당,,
<styled.p
      className={
        (css({
          textStyle: 'body1',
          color: 'primary',
        }),
        )
      }
    >
     text
    </styled.p>

@@ -3,7 +3,8 @@
"version": "0.0.0",
"private": true,
"exports": {
".": "./src/components/index.ts"
".": "./src/components/index.ts",
"./styles.css": "./dist/styles.css"
Copy link
Collaborator

Choose a reason for hiding this comment

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

just ask;
style.css의 경로가 이렇게 되어 있으면 packages/ui에 스타일에 대한 변경 사항이 생길때마다 build를 해주어야 할 것 같아요!
그런데 package.json에는 빌드 스크립트가 없는 것 같아 요거 추가하신 이유가 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

layout pr 에서 서현님께서 cssgen 하는 스크립트 만들어주셔서 요 PR 에서는 추가하진 않았습니다!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 그렇군요~!! 화긴화긴 🫡

Copy link
Collaborator

Choose a reason for hiding this comment

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

아직 dev 변경 사항이 반영 안 된 거 같은데 "./styles.css": "./src/styles.css"로 변경해뒀습니다!
그냥 ./src/index.css만 불러오면 스타일 적용이 안 돼서 cssgen 결과물을 import하는 방식으로 써야 할 거 같아요...
그래서 절대 경로 이슈도 있고, 이 이슈도 있어서 ui 패키지 빌드하면 좋을 거 같은...

Copy link
Collaborator

Choose a reason for hiding this comment

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

cssgen 결과물 import라...테스트는 storybook에서 진행하고 결과물 빌드해서 쓰는게 더 나을거같네용,,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋아용 이번 주 회의 때 이야기 나누고 이슈 파서 따로 작업해볼게요!

css: [
{
properties: {
textStyle: Object.keys(typography),
Copy link
Collaborator

Choose a reason for hiding this comment

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

just ask;
동적으로 컬러 지정하기 위해서는 컬러도 staticCss로 지정해야 할 것 같은데 그렇게 설정하지 않아도 괜찮나용?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wow-theme preset 정의하면서 staticCss 에서 color 부분은 정의해두었는데요! 타이포도 wow-theme preset 에서 한번에 정의하는 게 나을랑가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그렇구나 요거 wow-theme preset도 한번에 해주면 좋겠는데 요건 마이너 이슈이니 시간날떄 하면 좋을 거같아요!
덤으로 타이포 staticCSS 생성 스크립트도 쓰면 굳일거같긴합니닷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넹 말씀하신 건 이슈에 달아둘게요!

Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿 수고하셨습니다!

Comment on lines 25 to 26
<styled.p
className={
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
as 속성만 하나 추가해서 p 태그 말고도 h1, h2... 이런 태그를 받아도 좋을 거 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋아용 요거 반영해두었습니다!

@@ -3,7 +3,8 @@
"version": "0.0.0",
"private": true,
"exports": {
".": "./src/components/index.ts"
".": "./src/components/index.ts",
"./styles.css": "./dist/styles.css"
Copy link
Collaborator

Choose a reason for hiding this comment

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

아직 dev 변경 사항이 반영 안 된 거 같은데 "./styles.css": "./src/styles.css"로 변경해뒀습니다!
그냥 ./src/index.css만 불러오면 스타일 적용이 안 돼서 cssgen 결과물을 import하는 방식으로 써야 할 거 같아요...
그래서 절대 경로 이슈도 있고, 이 이슈도 있어서 ui 패키지 빌드하면 좋을 거 같은...

Copy link

@SeieunYoo
Copy link
Collaborator Author

리뷰 전부 반영해두었어요!

아래 부분들도 추가적으로 반영되었습니당

  • clsx 로 클래스네임 관리
  • panda-config 에서 타이포를 포함한 staticCss 를 관리하도록 수정 (추후 wowds-theme 에서 한번에 관리하는 것으로 수정하기)
  • NavItem 에 Text 컴포넌트 사용

@SeieunYoo SeieunYoo requested a review from ghdtjgus76 August 13, 2024 08:03
Copy link

Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿 수고하셨습니다!

@SeieunYoo SeieunYoo merged commit 0f11e37 into dev Aug 14, 2024
3 checks passed
@SeieunYoo SeieunYoo deleted the feat/text-component branch August 14, 2024 02:20
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