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

[LDS-9] remake custom chip component and story #75

Merged
merged 17 commits into from Jan 13, 2023
Merged

Conversation

HyejinYang
Copy link
Contributor

@HyejinYang HyejinYang commented Jan 11, 2023

Description

  • 피그마 링크
  • JIRA 이슈
  • 유선 회의를 통해 Chip의 상태가 변경된 부분을 적용해 작업 올립니다.
  • (참고) 현재 Figma와 컬러 시스템의 Chip 관련 컬러명이 1:1 매칭하지 않습니다.(지난 번 논의 후로 Figma에서 컬러 명칭이 바뀌었으나 DS 개발에는 아직 미적용된 상태) 또한, Hover 시의 컬러로 core.hover를 적용해두었으나 해당 기능이 완료되지 않은 상황이라 Figma와는 다르게 보일 것입니다.

Review Request

  • Chip의 상태가 Figma와 잘 매칭되는가
  • Chip의 interface를 상당 부분 바꾸었는데 괜찮은 방향인가
  • 그 외에 설계 혹은 기능 상 문제가 없는가

Additional works

  • 컬러 시스템 완성 후 변경된 색상 명칭 재작업
  • 컬러 시스템 완성 후 hover 시의 색상 변경 확인

@HyejinYang HyejinYang added the enhancement New feature or request label Jan 11, 2023
@HyejinYang HyejinYang requested review from deminoth, Jia1776 and a team January 11, 2023 09:33
@HyejinYang HyejinYang self-assigned this Jan 11, 2023
@github-actions
Copy link

github-actions bot commented Jan 11, 2023

@HyejinYang
Copy link
Contributor Author

오늘 회의에서 논의한 대로 전체 control에 Color BaseClass가 추가되면 Chip에서 decorators 부분 제거하겠습니다.

control: {
type: "select",
},
options: [
Copy link
Member

Choose a reason for hiding this comment

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

thumbnail을 한번 설정하면 뺄 수가 없으니 undefined도 있으면 좋겠습니당

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 undefined도 추가했습니다.
하는 김에 thumbnail icon은 variant가 "outlined"일 때와 "filled"일 때 둘 다를 볼 수 있도록 예시 추가했습니다.

@deminoth
Copy link
Member

색이 figma와 다르니 상태 리뷰가 쉽지 않네요 ㅋㅋ

fontStyle: "normal",
fontWeight: 500,
fontSize: "12px",
lineHeight: "14.32px",
Copy link
Contributor

Choose a reason for hiding this comment

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

브라우저마다 조금씩은 다르지만 CSS에서 소수점 단위 픽셀은 반올림 처리가 되기 때문에 생략하는게 좋지않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 잊고 있었는데 감사합니다. 소수점 삭제했습니다!

Copy link
Member

Choose a reason for hiding this comment

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

엇 그런데 피그마 inspect에서 나오는 css는 line-height: 16px; /* identical to box height, or 133% */ 이네용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

엇... 그새 수정되었었나 보네요. 안 그래도 어차피 텍스트는 중앙에 놓이는데 lineHeight가 14px일 이유가 없네 의아했습니다.
꼼꼼히 봐주셔서 감사해요. 수정했습니다!

Comment on lines 39 to 43
if (thumbnail && typeof thumbnail === "string")
return <Avatar>{thumbnail.slice(0, 1).toLocaleUpperCase()}</Avatar>;
if (thumbnail && typeof thumbnail === "string" && thumbnail.length === 0)
return <Avatar></Avatar>;
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (thumbnail && typeof thumbnail === "string")
return <Avatar>{thumbnail.slice(0, 1).toLocaleUpperCase()}</Avatar>;
if (thumbnail && typeof thumbnail === "string" && thumbnail.length === 0)
return <Avatar></Avatar>;
return undefined;
if (!thumbnail || typeof thumbnail !== "string") return;
if (thumbnail.length === 0) return <Avatar></Avatar>;
return <Avatar>{thumbnail.slice(0, 1).toLocaleUpperCase()}</Avatar>;

이런식으로 if 문 최적화 할 수 있을 것 같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

instance 아닌 이상 return 값은 어차피 undefined인 거 잊고 있었네요. 상세한 예시와 함께 최적화 방법 제안해주셔서 감사합니다!
반영했습니다!

@HyejinYang
Copy link
Contributor Author

HyejinYang commented Jan 13, 2023

유선 상 논의했던 onDelete 관련 문제는 Storybook 7으로 업그레이드를 진행하면 고쳐질 것으로 예상합니다.
업그레이드 진행 후에 ContainedWithDelete 스토리북 설정도 수정하겠습니다. (cc. @deminoth )

참고 자료: storybookjs/storybook#17063

@HyejinYang HyejinYang merged commit 8fd01d4 into main Jan 13, 2023
@HyejinYang HyejinYang deleted the chip-component branch January 13, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants