-
Notifications
You must be signed in to change notification settings - Fork 7
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
사용자 프로필 바 컴포넌트 구현 #152
사용자 프로필 바 컴포넌트 구현 #152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 ~ 사소한 부분을 찾아서 댓글로 남겼어요 :)
interface ProfileCardProps { | ||
profile: Participation; | ||
} | ||
export default function ProfileBox(props: ProfileCardProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProfileBox -> ProfileCard
컴포넌트 이름이 수정이 안돼있어요!!!!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 수정했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우리 스타일링을 위한 변수 네이밍에 대한 컨벤션을 두면 좋을 것 같아요~
저는 -Style
이렇게 뒀었어요.
이렇게 하자는 건 아니고, 정해지기만 하면 좋겠습니다 :0
지금 생각해보니 사용처에서 S.-
이렇게 쓰니까 -Style
suffix도 빼는 게 좋겠네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 한번 이야기 해봐요
import { css } from '@emotion/react'; | ||
|
||
export const ProfileContanier = css` | ||
scrollbar-width: none; /* Firefox 현재 no-unsupported-browser-features 경고 발생 중 추후 확인 필요 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석에 TODO
적어두면 나중에 별도로 모아서 확인할 수 있습니다!
@@ -6,6 +6,11 @@ declare module '*.woff2' { | |||
const src: string; | |||
export default src; | |||
} | |||
|
|||
declare module '*.svg?url' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
align-items: center; | ||
justify-content: center; | ||
|
||
width: ${width}rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 아예 문자에 rem을 박아버렸는데요
예를 들어 width:'7rem'과 같이요
이 부분을 조정해보면 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
밑에 width을 계산하는 부분이 있어서 rem으로 받으면 rem을 분리하고 숫자로 만든 다음 계산을 해야해서 일단 구현의 편의상 이와 같은 방식으로 구현했습니다. 다음 구현때 수정하도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function ProfileFrame(props: ProfileFrameProps) { | ||
const { width, height, onError, role, ...args } = props; | ||
|
||
const handleError = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 이게 싫어서 빈 프로필을 png로 두었어요.
이미지가 없는 에러가 나면 이미지 에러=>js 단에서 onError 수행=>이미지 변경된 컴포넌트 변경=>재렌더링=>재패칭 인데
background로 url을 두 개 두게되면
css에서 이미지 에러=>바로 재패칭 이렇게 되니깐요
물론 svg가 가벼워서 이 방식이 처음에는 더 이득일 수도 있겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 한번 다시 생각해볼 필요가 있겠네요. 아마 지금은 svg를 url로 사용할 수 있으니 가능할 것 같아요
export const Default: Story = { | ||
args: { | ||
participants: [ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 이 부분을 상수화해서 배열에 우겨넣으면 가독성이 좋더라구요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그런 방법도 있네요. refact 과정에 반영하도록 하겠습니다.
PR의 목적이 무엇인가요?
프로필 사진의 액자 컴포넌트(ProfileFrame)와 프로필 사진의 액자 컴포넌트와 사용자 이름을 합친 ProfileBox를 만들었습니다,
또한 ProfileBox의 리스트를 만드는 ProfileList를 구현했습니다.
SVG처리를 위해 storybook의 main.ts, webpack.common.js와 custom.d.ts를 수정하였습니다.
SVG를 사용할 때
위의 경우는 컴포넌트로 import하는 경우고
위의 경우는 문자열로 import하는 경우이다.
이슈 ID는 무엇인가요?
설명
질문 혹은 공유 사항 (Optional)